ebevhan added inline comments.
================ Comment at: lib/Parse/ParseDeclCXX.cpp:2313 + } + } ---------------- Anastasia wrote: > 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. :) Yes, keyword based ones like in OpenCL. Basically, in our downstream we look for keywords in all of the qualifier/specifier parsing routines (`ParseTypeQualifierListOpt`, `ParseDeclarationSpecifiers`) and apply an artificial `address_space` attribute to the DeclSpec being parsed. This has the same effect as using a regular AS attribute, but it's applied with a keyword instead. The parsing in ParseCXXMemberDeclaratorBeforeInitializer cannot parse anything but physical GNU address_space attributes, so it can't be used to parse these. It would work for us if the AS application part (that calls `asOpenCLLangAS`) in ParseFunctionDeclarator looked for both the OpenCL AS attributes and AT_AddressSpace, but it's hard to justify looking for AT_AddressSpace there since the parser will never actually parse those (it's disabled with AR_NoAttributesParsed). I suppose a solution could be to rename `asOpenCLLangAS` and have it return ASes from regular AT_AddressSpace attributes as well? 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
