You are right, we need to process two tokens at once. Good test case :)<br>Attach updated patch, please take a look. Thanks!<br><br>Cheers<br>Michael<br><br><div class="gmail_quote">On Tue, Nov 20, 2012 at 4:03 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Mon, Nov 19, 2012 at 11:13 AM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>> wrote:<br>
> Thanks! Here is the updated patch that:<br>
> - Enable ParseImplicitInt to pass attributes to caller.<br>
> - Handle alignment specifiers as well.<br>
> - Fix comments and variable naming Sean pointed out.<br>
> - Update test case.<br>
<br>
</div>+ // Skip C++11 attribute specifiers.<br>
+ do {<br>
+ if (Tok.is(tok::l_square))<br>
+ ConsumeBracket();<br>
+ else<br>
+ ConsumeToken();<br>
+<br>
+ if (Tok.is(tok::l_square)) {<br>
<div class="im">+ ConsumeBracket();<br>
+ SkipUntil(tok::r_square, false);<br>
+ SkipUntil(tok::r_square, false);<br>
</div>+ }<br>
+<br>
+ if (Tok.is(tok::l_paren)) {<br>
+ ConsumeParen();<br>
+ SkipUntil(tok::r_paren);<br>
+ }<br>
+ } while (Tok.is(tok::l_square) || Tok.is(tok::kw_alignas) ||<br>
+ Tok.is(tok::kw__Alignas));<br>
<br>
This doesn't look right. For instance, it looks like it will classify<br>
'struct S final [(int){0}];' as a definition. How about something more<br>
like:<br>
<br>
while (true) {<br>
if (Tok.is(tok::l_square) && NextToken().is(tok::l_square)) {<br>
ConsumeBracket();<br>
if (!SkipUntil(tok::r_square))<br>
break;<br>
} else if ((Tok.is(tok::kw_alignas) || Tok.is(tok::kw__Alignas)) &&<br>
NextToken().is(tok::l_paren)) {<br>
ConsumeToken();<br>
ConsumeParen();<br>
if (!SkipUntil(tok::r_paren))<br>
break;<br>
} else {<br>
break;<br>
<div class="HOEnZb"><div class="h5"> }<br>
}<br>
<br>
> On Sun, Nov 18, 2012 at 7:00 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> Index: lib/Parse/ParseDecl.cpp<br>
>> ===================================================================<br>
>> --- lib/Parse/ParseDecl.cpp (revision 168292)<br>
>> +++ lib/Parse/ParseDecl.cpp (working copy)<br>
>> @@ -1923,12 +1923,13 @@<br>
>> << TokenName << TagName;<br>
>> }<br>
>><br>
>> + ParsedAttributesWithRange attrs(AttrFactory);<br>
>> // Parse this as a tag as if the missing tag were present.<br>
>> if (TagKind == tok::kw_enum)<br>
>> ParseEnumSpecifier(Loc, DS, TemplateInfo, AS, DSC_normal);<br>
>> else<br>
>> ParseClassSpecifier(TagKind, Loc, DS, TemplateInfo, AS,<br>
>> - /*EnteringContext*/ false, DSC_normal);<br>
>> + /*EnteringContext*/ false, DSC_normal,<br>
>> attrs);<br>
>> return true;<br>
>> }<br>
>><br>
>> It looks like the attributes are being discarded here. This is an<br>
>> error path, but we have a fix-it so we still need to recover properly.<br>
>> It'd be fine to add an extra parameter to ParseImplicitInt for this.<br>
>><br>
>> @@ -1234,12 +1235,24 @@<br>
>> // - If we have 'struct foo;', then this is either a forward<br>
>> declaration<br>
>> // or a friend declaration, which have to be treated differently.<br>
>> // - Otherwise we have something like 'struct foo xyz', a reference.<br>
>> + // When start parsing from here, we also take misplaced C++11<br>
>> attributes<br>
>> + // specifiers into consideration by detecting the following cases:<br>
>> + // - attributes follow class-name:<br>
>> + // struct foo [[]] {};<br>
>> + // - attributes appear before or after 'final':<br>
>> + // struct foo [[]] final [[]] {};<br>
>> + // This allow we produce better diagnostics.<br>
>><br>
>> Something funny seems to have happened to the grammar in this comment.<br>
>><br>
>> @@ -1261,6 +1274,30 @@<br>
>> // Okay, this is a class definition.<br>
>> TUK = Sema::TUK_Definition;<br>
>> }<br>
>> + } else if (isCXX0XFinalKeyword() && (NextToken().is(tok::l_square))) {<br>
>> + // We can't tell if this is a declaration or definition or reference<br>
>> + // until we skipped the 'final' and attributes sequences.<br>
>><br>
>> This comment isn't quite right: this can't be a declaration if we have<br>
>> the 'final' keyword here.<br>
>><br>
>> + do {<br>
>> + // Skip the attributes without parsing them<br>
>> + if (NextToken().is(tok::l_square)) {<br>
>> + ConsumeBracket();<br>
>> + ConsumeBracket();<br>
>> + SkipUntil(tok::r_square, false);<br>
>> + SkipUntil(tok::r_square, false);<br>
>><br>
>> The indentation here is off.<br>
>><br>
>> + }<br>
>> + } while (Tok.is(tok::l_square));<br>
>><br>
>> It'd be good to handle alignment-specifiers (the other kind of C++11<br>
>> attribute-specifier) here.<br>
>><br>
>> On Sun, Nov 18, 2012 at 3:52 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br>
>> > + ParsedAttributesWithRange attrs(AttrFactory);<br>
>> ><br>
>> > Variable names should be uppercase, as per the coding standards:<br>
>> ><br>
>> > <<a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a>><br>
>> ><br>
>> > -- Sean Silva<br>
>> ><br>
>> > On Sun, Nov 18, 2012 at 5:52 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> > wrote:<br>
>> >> Hi Richard,<br>
>> >><br>
>> >> Thanks for the suggestions!<br>
>> >><br>
>> >> Attached updated patch. The parser now will parse C++11 attribute<br>
>> >> specifiers, in the form of double squares that appear at all possible<br>
>> >> syntactic locations following class-name in a class specifier (also<br>
>> >> before<br>
>> >> or after 'final' keyword). In case of final keyword, I have not found a<br>
>> >> way<br>
>> >> to classify the nature of the class specifier without using tentative<br>
>> >> parsing. Also updated test cases.<br>
>> >><br>
>> >> I will look into fix-it in a separate patch.<br>
>> >><br>
>> >> Cheers<br>
>> >> Michael<br>
>> >><br>
>> >><br>
>> >> On Thu, Nov 15, 2012 at 10:32 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> On Thu, Nov 15, 2012 at 10:33 AM, Michael Han<br>
>> >>> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >>> wrote:<br>
>> >>> > Hi,<br>
>> >>> ><br>
>> >>> > Consider this code<br>
>> >>> > class [[foo]] c [[foo]];<br>
>> >>> ><br>
>> >>> > Clang generates diagnostic like this:<br>
>> >>> > error: an attribute list cannot appear here<br>
>> >>> > class [[foo]] c [[foo]];<br>
>> >>> > ^~~~<br>
>> >>> ><br>
>> >>> > I think the first attribute is conforming, and it's the second<br>
>> >>> > attribute<br>
>> >>> > that should be diagnosed.<br>
>> >>><br>
>> >>> Yes, I agree that it would be better to point the diagnostic at the<br>
>> >>> second attribute-specifier-seq (and ideally to say that it's in the<br>
>> >>> wrong place, and offer a fixit...).<br>
>> >>><br>
>> >>> > Attached the patch that fixes this.<br>
>> >>><br>
>> >>> I don't think your approach here is going to work. This is valid:<br>
>> >>><br>
>> >>> class c {};<br>
>> >>> class c [[ ]] x;<br>
>> >>><br>
>> >>> I believe your patch would treat 'class c' as a declaration in the<br>
>> >>> second line, which it is not.<br>
>> >>><br>
>> >>> In order to correctly handle this, I suggest you parse attributes<br>
>> >>> after the class name as well as before it, before trying to classify<br>
>> >>> the reference. If you then find that it's a TUK_Reference, return the<br>
>> >>> attributes you found after the class name to the caller, for them to<br>
>> >>> handle appropriately. Otherwise, produce a diagnostic indicating that<br>
>> >>> they were written in the wrong place (offering a fixit would be<br>
>> >>> awesome...). For a class definition, you should probably look for<br>
>> >>> attributes both before and after the optional 'final'.<br>
>> >>><br>
>> >>> Thanks for looking into this!<br>
>> >><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-commits mailing list<br>
>> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >><br>
><br>
><br>
</div></div></blockquote></div><br>