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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits