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

[Nico Weber via cfe-commits]

thakis added inline comments.

================
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:
> 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).
I too feel a bit lost :-)

Right now, the parsing code doesn't look at attribute lists, only at tokens, so `[identify_class_by(uuid),` skips tokens until we hit 'uuid', and if I were to diag on not-'(' after 'uuid', then I'd diag here, right? Do you want me to do some more parsing to look for comma-separated entities in the attribute list (and count parens brackets braces and ignore commas there) to fix this?

Sorry, I feel we're talking past each other :-(


https://reviews.llvm.org/D23895