[PATCH] D23895: [ms] Add support for parsing uuid as a MS attribute

[Nico Weber via cfe-commits]

thakis marked an inline comment as done.
thakis added a comment.

Thanks for the fast review and the good comments! I'll leave it to y'all to agree on some name if you don't like the one I picked.

> Also, doesn't this introduce ambiguities into the grammar? Something like this:

> 

>   void useit(int);

>   int main() {

>     int uuid = 42;

>     [uuid]() {

>       useit(uuid);

>     }();

>   }

>    

> 

> Will we keep parsing that as a lambda after this change or not?


This change shouldn't change how things are parsed. It moved calls of MaybeParseMicrosoftAttributes(attrs) that precede ParseExternalDeclaration(attrs) into ParseExternalDeclaration(). As far as I can tell, every call of ParseExternalDeclaration() was preceded by a call to MaybeParseMicrosoftAttributes(), so this should be behavior-preserving. In your example, the lamda is not an external declaration, so it's not affected. (That means [uuid(...)] won't work for local classes, but that's probably ok.)

Now that I looked at this more, behavior changes slightly for empty external declarations. Before,

[foob];

didn't produce any output, now it produces "declaration doesn't declare anything". I don't know if that's important.


================
Comment at: include/clang/Basic/Attr.td:201
@@ -200,2 +200,3 @@
 class Declspec<string name> : Spelling<name, "Declspec">;
+class MS<string name> : Spelling<name, "MS">;
 class CXX11<string namespace, string name, int version = 1>
----------------
rsmith wrote:
> Is there some better description for this than MS? `__declspec` is also an MS attribute. Is it fair to call this IDL or MSIDL or similar?
There are many IDL compilers. MSIDL works for me, so does anything else you 3 agree on :-)

================
Comment at: include/clang/Parse/Parser.h:2109-2110
@@ -2108,4 +2108,4 @@
 
-  void handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange &Attrs,
-                                         DeclSpec &DS, Sema::TagUseKind TUK);
+  void stripSomeAttribsOffDeclSpec(ParsedAttributesWithRange &Attrs,
+                                   DeclSpec &DS, Sema::TagUseKind TUK);
 
----------------
rsmith wrote:
> Can you give this a less vague-sounding name? :)
Changed to stripTypeAttribsOffDeclSpec

================
Comment at: lib/Parse/ParseDeclCXX.cpp:3950
@@ +3949,3 @@
+      ConsumeToken();
+      if (Name->getName() == "uuid")
+        ParseMicrosoftDeclSpecArgs(Name, Loc, attrs, AttributeList::AS_MS);
----------------
rsmith wrote:
> Is anything known to fail if you accept arbitrary attributes here, or is this just the only one you /know/ you need right now? (If we can ditch the whitelist, that would seem preferable.)
These attributes can be different from decl spec args. For example, we don't have anything that parses `threading("apartment")`, `module(name="MyLib")` at the moment (I suppose ParseAttributeArgsCommon does that at a token level, but it won't know all these attributes that are allowed here).


https://reviews.llvm.org/D23895