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

[Nico Weber via cfe-commits]

thakis added inline comments.

================
Comment at: include/clang/Parse/Parser.h:2217
@@ -2215,1 +2216,3 @@
+                                       SourceLocation UuidLoc,
+                                       ParsedAttributes &attrs);
   void ParseMicrosoftAttributes(ParsedAttributes &attrs,
----------------
aaron.ballman wrote:
> `Attrs` instead of `attrs`
It's spelled `attrs` in the function above and below this one…

================
Comment at: lib/Parse/ParseDeclCXX.cpp:3947
@@ +3946,3 @@
+
+    SmallString<42> StrBuffer; // 2 "", 36 bytes UUID, 2 optional {}, 1 nul
+    StrBuffer += "\"";
----------------
aaron.ballman wrote:
> Should we use Twine for building this string up (since this is used in a lot of header files)?
I thought Twine is a rvalue type…yes, from its docs:

```
  /// A Twine is not intended for use directly and should not be stored, its
  /// implementation relies on the ability to store pointers to temporary stack
  /// objects which may be deallocated at the end of a statement. Twines should
  /// only be used accepted as const references in arguments, when an API wishes
  /// to accept possibly-concatenated strings.
```

I think this is should be pretty fast as-is.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:3993
@@ +3992,3 @@
+    StringLiteral *UuidString =
+        cast<StringLiteral>(Actions.ActOnStringLiteral(Toks, nullptr).get());
+    ArgExprs.push_back(UuidString);
----------------
aaron.ballman wrote:
> Will this properly handle an abomination involving string literal concatenation? I shudder to type this, but: `[uuid({000000A0-0000-""0000-C000-000000000049})] struct S {};` MSVC rejects, but I think this code will silently accept it.
This is rejected, "" is returned as a string token which becomes `""` (including quotes) after getSpelling. Added a test case with this.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:4031
@@ +4030,3 @@
+      ConsumeToken();
+      if (Name->getName() == "uuid" && Tok.is(tok::l_paren))
+        ParseMicrosoftUuidAttributeArgs(Name, Loc, attrs);
----------------
aaron.ballman wrote:
> How well does this diagnose `[uuid{"000000A0-0000-0000-C000-000000000049"}] struct S {};`? (Note the use of curly braces in lieu of parens for the argument.)
That's currently silently ignored since ParseMicrosoftAttributes() looks for 'uuid' '(' to call the uuid parser.

================
Comment at: test/Parser/ms-square-bracket-attributes.mm:12
@@ +11,3 @@
+}
+
+
----------------
aaron.ballman wrote:
> Can we also get a test case showing the behavior of multiple arguments in the same uuid attribute? e.g., `[uuid("{000000A0-0000-0000-C000-000000000049}", "1")] struct S {};`
> And a test showing that we reject multiple uuid attributes on the same entity? e.g., `[uuid("{000000A0-0000-0000-C000-000000000049}"), uuid("{000000A0-0000-0000-C000-000000000050}")] struct S {};`
The first is a nice testcase, added, thanks.

We don't currently reject multiple uuids (also not with declspec). Fixing this is on my list of follow-ups, but since the fix will live in sema land and be independent of the attribute spelling, I didn't put it in this change. (Fun fact: cl.exe doesn't warn if you have one []-uuid and one declspec-uuid. We'll get that right once we diag on it.)


https://reviews.llvm.org/D23895