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

Fri Feb 1 03:49:55 PST 2019

Anastasia marked 2 inline comments as done.
Anastasia added inline comments.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:2313
+    }
+  }

----------------
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!

================
Comment at: test/SemaCXX/address-space-method-overloading.cpp:28
+   //inas4.bar();
+   noas.bar(); // expected-error{{call to member function 'bar' is ambiguous}}
+}
----------------
ebevhan wrote:
> Just as an aside (I think I mentioned this on one of the earlier patches as well); treating a non-AS-qualified object as being in a 'generic' AS even for normal C++ isn't a great idea IMO. The object initialization code should be checking if the ASes of the actual object and the desired object type are compatible, and only if so, permit the overload.
I think changing this would require changing AS compatibility rules in general, not just for overloading but for example for conversions too. This seems like a wider scope change that might affect backwards compatibility. We might need to start an RFC on this first. John has suggested adding a target setting  for this on the other review. I think that should work.  Another idea could be to add some compiler directives to specify what generic address space is (if any).

Using default (no address space) as generic has a lot of advantages since it doesn't need many parser changes. In OpenCL we weren't lucky that initial implementation was added that used default for private memory and therefore when generic was introduced we had to map it to a new lang addr space value. That required a lot of changes in the parser. But once we fix those actually, anyone should be able  to map generic to anything. I initially thought it has no other use cases than in OpenCL but now I feel there might be a value to map default case (no address space) for something specific and then use generic to specify a placeholder address space on pointers or references. We could just expose generic address space generically and also have a mode with no generic address space. The latter means that conversions between different address spaces is never valid. Would this make sense?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57464/new/

https://reviews.llvm.org/D57464