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

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

You are removing a lot of instances of `MaybeParseMicrosoftAttributes()`, but I'm not certain I understand why. Can you describe why you need to remove those?

There are things missing from this patch as well. To wit:

- Attributes.h needs to know about this attribute syntax, and `hasAttribute()` needs to support it (this hooks into `__has_attribute` support).
- AttributeList.h should be updated to expose the parsed attribute syntax.
- Both of these changes should be threaded through ClangAttrEmitter.cpp.
- We should consider adding (perhaps as a separate patch?) `__has_microsoft_attribute()` to complement `__has_declspec_attribute()`.
- This change should probably go into the release notes.

I agree with @rnk, I would like to see more comprehensive tests of what we do and do not support for the parsing of these attributes. Previously, we didn't support Microsoft attributes at all, so it was fine to just eat them whenever they appeared and not worry about whether we were actually parsing them properly or not. Now that we want to support them, I want to make sure the parsing is correct, even if uuid wouldn't show up in that particular position. I am fine if your tests show things we don't parse properly yet with FIXME comments (assuming that your patch does not regress that behavior). However, I'd prefer those tests appear in Parser rather than SemaCXX. ;-)


================
Comment at: include/clang/Parse/Parser.h:2109
@@ -2108,3 +2108,3 @@
 
-  void handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange &Attrs,
-                                         DeclSpec &DS, Sema::TagUseKind TUK);
+  void stripTypeAttribsOffDeclSpec(ParsedAttributesWithRange &Attrs,
+                                   DeclSpec &DS, Sema::TagUseKind TUK);
----------------
Better, but how about `stripTypeAttributesOffDeclSpec`? We don't use "attribs" anywhere else.

================
Comment at: include/clang/Sema/AttributeList.h:111
@@ -108,3 +110,3 @@
     /// #pragma ...
-    AS_Pragma
+    AS_Pragma,
   };
----------------
Please remove the trailing comma.

================
Comment at: lib/Parse/ParseDecl.cpp:516-517
@@ -514,5 +515,4 @@
 
-  unsigned NumArgs =
-      ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, nullptr, nullptr,
-                               SourceLocation(), AttributeList::AS_Declspec);
+  unsigned NumArgs = ParseAttributeArgsCommon(
+      AttrName, AttrNameLoc, Attrs, nullptr, nullptr, SourceLocation(), AS);
 
----------------
This change is not incorrect, but I think you need a custom parsing step as well. IIRC, the uuid attribute supports unquoted GUIDs, like `[uuid(A-B-C-D)]`, and this won't handle it properly. Attr.td says UUID takes a `StringArgument`, which may require a bit of thought as to whether we need a flag on there to say "or... maybe not a string...for Microsoft attributes only". Perhaps other MS attributes do the same thing and we're okay for our declarative information?

================
Comment at: lib/Parse/ParseDecl.cpp:1427-1428
@@ -1425,2 +1426,4 @@
 
+// Usually, `__attribute__((attrib)) class Foo {} var` means that attrib applies
+// to var, not the type Foo.
 // As an exception to the rule, __declspec(align(...)) before the
----------------
I'd spell this out a bit more.
```
// Usually, `__attribute__((attrib)) class Foo {} var;` means that the attribute appertains to the declaration (var) rather than the type (Foo).
```


================
Comment at: lib/Parse/ParseDecl.cpp:1431
@@ -1427,4 +1430,3 @@
 // class-key affects the type instead of the variable.
-void Parser::handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange &Attrs,
-                                               DeclSpec &DS,
-                                               Sema::TagUseKind TUK) {
+// Also, Microsoft-style [attributes] affect the type instead of the variable.
+// This function moves attributes that should apply to the type off DS to Attrs.
----------------
Is this documented somewhere, or just what you gather from experimentation?

================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:2372
@@ -2363,3 +2371,3 @@
                 .Case("Declspec", 2)
-                .Case("Keyword", 3)
-                .Case("Pragma", 4)
+                .Case("Microsoft", 3)
+                .Case("Keyword", 4)
----------------
I think (but my memory is a bit fuzzy here) that these values need to match the ones from AttributeList.h. I think it's a bug that Pragma was at 4 rather than 5, but since __has_attribute doesn't care about pragmas (or context sensitive keywords), it's likely benign.


https://reviews.llvm.org/D23895