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

[Anastasia Stulova via Phabricator via cfe-commits]

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


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2309
+                                  attr.getScopeName(), attr.getScopeLoc(), &AU,
+                                  attr.getNumArgs(), ParsedAttr::AS_GNU);
+        attr.setInvalid();
----------------
ebevhan wrote:
> Are we interested in preserving the spelling?
I think it is computed based on the properties of `ParsedAttr` (like Name or Syntax) https://clang.llvm.org/doxygen/classclang_1_1ParsedAttr.html#a5e53f185ac34c208f6e734d6d7103d8c.

At least I don't see any member that stores this in `ParsedAttr`.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2313
+    }
+  }
 
----------------
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...


================
Comment at: lib/Sema/SemaType.cpp:4883
+                          return AS;
+                        }};
+          LangAS AS = LangAS::Default;
----------------
ebevhan wrote:
> This lambda is a bit unwieldy, it would probably be better as a separate function.
Sure! I will rewrite this with just a lang mode condition inside the loop. I was trying to avoid this extra check inside the loop but perhaps complexity is not worth it. This code is full of checks anyway.





================
Comment at: lib/Sema/SemaType.cpp:5865
+/// value and argument expression will be filled in. Returns value indicating
+/// whether any diagnostic occurred.
+static bool ProcessAddressSpaceAttribute(Sema &S, const ParsedAttr &Attr,
----------------
ebevhan wrote:
> "Returns value" could be more specific. "Returns true if the attribute was processed successfully."
Sure! Thanks!


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

https://reviews.llvm.org/D57464