[PATCH] D57464: Generalize method overloading on addr spaces to C++

[Bevin Hansson via Phabricator via cfe-commits]

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