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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to