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: > > > > 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. 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
