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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/Attr.td:201
@@ -200,2 +200,3 @@
 class Declspec<string name> : Spelling<name, "Declspec">;
+class Microsoft<string name> : Spelling<name, "Microsoft">;
 class CXX11<string namespace, string name, int version = 1>
----------------
thakis wrote:
> rsmith wrote:
> > Given that MS have deprecated this syntax and are trying to move away from it towards `__declspec`, I think calling this "Microsoft" is a mistake. As MS' documentation calls these "ATL attributes", it would seem reasonable for us to do the same.
> As said upthread, I'm happy to rename this to whatever you all agree on (or what you decree given that you're code owner), but they are called Microsoft attributes in today's code already, and renaming that is unrelated to this CL. If you think it's important, I'm happy to rename the existing code before lading this CL and then using the new name in this CL immediately.
> 
> Re "ATL attributes": I think some of the attributes in [] are SAL attributes which as far as I know is independent of ATL. Are you sure the documentation refers to the [] syntax, or could it refer to specific attributes in that syntax?
MS' documentation does not call them "ATL attributes" only. The documentation also calls them COM attributes, compiler attributes, and other names. However, the compiler diagnoses them as "usage of ATL attributes is deprecated", so it's somewhat defensible as a name.

I think "ATL" is the wrong name for them *only here*, but I'm not opposed to making a sweeping change to name them ATL attributes everywhere in the compiler (including here). However, I also don't see an issue with continuing to call them Microsoft attributes, either, because it's unlikely to cause any lasting confusion (it's caused zero confusion in the years I've been working on Clang).

================
Comment at: lib/Parse/ParseDeclCXX.cpp:4031
@@ +4030,3 @@
+      ConsumeToken();
+      if (Name->getName() == "uuid" && Tok.is(tok::l_paren))
+        ParseMicrosoftUuidAttributeArgs(Name, Loc, attrs);
----------------
thakis wrote:
> aaron.ballman wrote:
> > thakis wrote:
> > > aaron.ballman wrote:
> > > > Silently ignoring seems like the wrong thing to do -- can we diagnose (even if it's a less-than-ideal diagnostics with a fixme)?
> > > We still skip the majority of [] contents. Maybe the token 'uuid' can appear in some other attribute, followed by something else. So we probably shouldn't diag on 'uuid' followed by not-'(' (right?). Do you want me to add a diagnostic for 'uuid' '{'? What about 'uuid' '['?
> > The grammar for the uuid attribute shows it requires the parens (it also shows that it only accepts a string literal, so take that with a grain of salt), so I think we should diagnose in this case, especially since we're manually parsing the args. So if you see "[uuid" followed by any token other than "(", I think that's an error (and MSVC treats it as such).
> Sure, but uuid could be preceded by other tokens, e.g.
> 
> [ someotherattrib(foobar...) ..., uuid {
> 
> someotherattrib might take an argument that's called uuid and it might be valid to have uuid followed by something not '(' there, say [identify_class_by(uuid), uuid("1-2-3")]. This is a made-up example; I don't know if this is actually the case, I'm just saying I don't know, so I think I shouldn't diag on 'uuid' followed by something not-'(' in general.
> 
> Do you want me to peephole-diag on '[' 'uuid' not-'(' in the case when uuid is the first attrib in the attrib list?
I'm a bit lost. You've processed the open square bracket, so you know you're in an attribute list. Then you ignore everything until you get an identifier. If that identifier is uuid, you do special processing. Right now, you require uuid( to do the special processing. What I am saying is that if, when processing an attribute-token within an attribute list, you find uuid followed by anything other than a (, it should be diagnosed.

So, I expect [identify_class_by(uuid), uuid("1-2-3")] to be fine, but [uuid["1-2-3-"], identify_by_class(uuid), uuid*] to diagnose the uuid["1-2-3"] and uuid* as being malformed (though I'd be fine if we skipped everything past the first malformed attribute if it's better for error recovery).


https://reviews.llvm.org/D23895