[cfe-commits] [Patch] Fix parser diagnostic on class specifiers following c++11 attributes
Michael Han
fragmentshaders at gmail.com
Fri Nov 23 15:55:10 PST 2012
Hi Richard,
Is the updated patch ok to commit?
Cheers
Michael
On Tue, Nov 20, 2012 at 7:10 PM, Michael Han <fragmentshaders at gmail.com>wrote:
> You are right, we need to process two tokens at once. Good test case :)
> Attach updated patch, please take a look. Thanks!
>
> Cheers
> Michael
>
>
> On Tue, Nov 20, 2012 at 4:03 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> 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
>> >> >>
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121123/f023e02a/attachment.html>
More information about the cfe-commits
mailing list