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