Anastasia marked an inline comment as done. Anastasia added inline comments.
================ Comment at: lib/Parse/ParseDeclCXX.cpp:2313 + } + } ---------------- ebevhan wrote: > Anastasia wrote: > > ebevhan wrote: > > > Anastasia wrote: > > > > ebevhan wrote: > > > > > Anastasia wrote: > > > > > > Anastasia wrote: > > > > > > > ebevhan wrote: > > > > > > > > Is there a reason that the attributes are parsed here and not > > > > > > > > in `ParseFunctionDeclarator` like the rest of the > > > > > > > > ref-qualifiers (and the OpenCL++ ASes)? > > > > > > > > > > > > > > > > That is possibly why the parser is getting confused with the > > > > > > > > trailing return. > > > > > > > Good question! I have a feeling that this was done to unify > > > > > > > parsing of all CXX members, not just methods. For collecting the > > > > > > > method qualifiers it would certainly help making flow more > > > > > > > coherent if this was moved to `ParseFunctionDeclarator`. I will > > > > > > > try to experiment with this a bit more. What I found strange that > > > > > > > the attributes here are attached to the function type itself > > > > > > > instead of its qualifiers. May be @rjmccall can shed some more > > > > > > > light on the overall flow... > > > > > > I looked at this a bit more and it seems that all the GNU > > > > > > attributes other than addr spaces belong to the function (not to be > > > > > > applied to `this`) for example > > > > > > https://blog.timac.org/2016/0716-constructor-and-destructor-attributes/. > > > > > > It seems correct to parse them here and apply to the function > > > > > > type. Although we could separate parsing of addr space attribute > > > > > > only and move into `ParseFunctionDeclarator`. However this will > > > > > > only be needed for the function type not for the data members. So > > > > > > not sure whether it will make the flow any cleaner. > > > > > > I looked at this a bit more and it seems that all the GNU > > > > > > attributes other than addr spaces belong to the function (not to be > > > > > > applied to this) > > > > > > > > > > Hm, I know what you mean. It's why Clang usually complains when you > > > > > try placing an AS attribute after a function declarator, because it's > > > > > trying to apply it to the function rather than the method qualifiers. > > > > > > > > > > > However this will only be needed for the function type not for the > > > > > > data members. > > > > > > > > > > But we aren't really interested in data members, are we? Like struct > > > > > fields, non-static data members cannot be qualified with ASes (they > > > > > should 'inherit' the AS from the object type), and static ones should > > > > > probably just accept ASes as part of the member type itself. > > > > > > > > > > These patches are to enable AS-qualifiers on methods, so it only > > > > > stands to reason that those ASes would be parsed along with the other > > > > > method qualifiers. > > > > > > > > > > ParseFunctionDeclarator calls ParseTypeQualifierListOpt to get the > > > > > cv-qualifier-seq for the method qualifiers. Currently it's set to > > > > > `AR_NoAttributesParsed`. If we enable parsing attributes there, then > > > > > the qualifier parsing might 'eat' attributes that were possibly meant > > > > > for the function. > > > > > > > > > > This is just a guess, but what I think you could do is include > > > > > attributes in the cv-qualifier parsing with `AR_GNUAttributesParsed`, > > > > > look for any AS attributes, take their AS and mark them invalid, then > > > > > move the attributes (somehow) from the qualifier-DeclSpec to the > > > > > `FnAttrs` function attribute list. > > > > > > > > > > (The reason I'm concerned about where/how the qualifiers are parsed > > > > > is because this approach only works for custom ASes applied with the > > > > > GNU attribute directly. It won't work if custom address spaces are > > > > > given with a custom keyword qualifier, like in OpenCL. If the ASes > > > > > are parsed into the DeclSpec used for the other ref-qualifiers, then > > > > > both of these cases should 'just work'.) > > > > > But we aren't really interested in data members, are we? Like struct > > > > > fields, non-static data members cannot be qualified with ASes (they > > > > > should 'inherit' the AS from the object type), and static ones should > > > > > probably just accept ASes as part of the member type itself. > > > > > > > > Pointer data members can be qualified by and address space, but this > > > > should be parsed before. > > > > > > > > > > > > > This is just a guess, but what I think you could do is include > > > > > attributes in the cv-qualifier parsing with AR_GNUAttributesParsed, > > > > > look for any AS attributes, take their AS and mark them invalid, then > > > > > move the attributes (somehow) from the qualifier-DeclSpec to the > > > > > FnAttrs function attribute list. > > > > > > > > Ok, I will try that. Thanks for sharing your ideas! > > > > > > > > Pointer data members can be qualified by and address space, but this > > > > should be parsed before. > > > > > > Right, pointer-to-member is a thing. I always forget about those. > > It seems unfortunately that moving parsing of GNU attributes into > > `ParseFunctionDeclarator` might not be a good viable alternative. One of > > the issues I encountered that some attributes accept class members, > > example fragment from **test/SemaCXX/warn-thread-safety-analysis.cpp**: > > > > > > ``` > > class LateFoo { > > > > ... > > > > void foo() __attribute__((requires_capability(mu))) { } > > > > ... > > > > Mutex mu; > > }; > > ``` > > > > > > As CXX name lookup is not setup in `ParseFunctionDeclarator` it seems > > unreasonable to replicate all these logic there. > > > > I have also tried to move just address space parsing only into > > `ParseFunctionDeclarator` but there is a corner cases of GNU attribute > > syntax where multiple attr are specified as a list: > > ``` > > __attribute__((overloadable, address_space(11))) > > ``` > > that means we can't always parse them separately. :( > > > > Based on this, it seems better to leave the parsing here in > > `ParseCXXMemberDeclaratorBeforeInitializer`. > > > > Can you explain a bit more why you prefer it to be in > > `ParseFunctionDeclarator` (other than avoiding two separate places with > > similar functionality that I do agree would be better to avoid)? > > > > > (The reason I'm concerned about where/how the qualifiers are parsed is > > > because this approach only works for custom ASes applied with the GNU > > > attribute directly. It won't work if custom address spaces are given with > > > a custom keyword qualifier, like in OpenCL. If the ASes are parsed into > > > the DeclSpec used for the other ref-qualifiers, then both of these cases > > > should 'just work'.) > > > > The keywords based address spaces can easily be handled in > > `ParseFunctionDeclarator` just like for OpenCL. If there is something I can > > generalize there I am happy to do. Just let me know. > > > > > > > Well, that's unfortunate... > > > As CXX name lookup is not setup in ParseFunctionDeclarator it seems > > unreasonable to replicate all these logic there. > > I think the issue is rather that attributes parsed during a class > construction are added to the `LateAttrs` list to be parsed after the class > is finished (to enable the member lookup), and there's currently no way (as > far as I can see) to get to the `LateAttrs` from `ParseFunctionDeclarator`. > Also, `ParseTypeQualifierListOpt` does not take a LateParsedAttrList to give > to its invocation of `ParseGNUAttributes` anyway. There's just no immediate > way to 'defer' the parsing of the attributes when parsing the function > declarator (because we don't necessarily have to be in class scope to be > parsing one). > > Only parsing AS attrs was an option, but that list syntax gets in the way... > > > Can you explain a bit more why you prefer it to be in > > ParseFunctionDeclarator (other than avoiding two separate places with > > similar functionality that I do agree would be better to avoid)? > > It's more or less a combination of > * not wanting code that does the same thing in multiple places > * parsing the AS qualifier along with the other method qualifiers seems like > The Right Thing to do; for example, it should solve the issue with the > misparse on trailing return > * only `__attribute__` ASes will be parsed, not specifier-based ones; it > could be solved by adding something that looks for AT_AddressSpace in > `ParseFunctionAttribute` even though the parser won't ever parse a real AS > attribute, but it doesn't feel like the cleanest solution > > Ultimately it boils down to the parser not being aware of the class for which > it is parsing a function declarator, so there's no way to tell it 'wait with > parsing this until the class is finished'. None of the existing method > qualifiers needs this, so there's no way of doing it (except for exception > specifications, but those are handled specially). > > I'm not really sure how to solve this in a way that makes me satisfied... > John maybe has a different opinion, though. Ok, agreed let's see if @rjmccall can suggest something. > only __attribute__ ASes will be parsed, not specifier-based ones; Do you refer to keyword-based ASes? Because this should work now after OpenCL was fixed. However, some generalization might be needed in several places to un-condition some code on OpenCL. I can try to look into it if I get an example code. :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57464/new/ https://reviews.llvm.org/D57464 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
