[cfe-commits] [Patch] Fix parser diagnostic on class specifiers following c++11 attributes
Michael Han
fragmentshaders at gmail.com
Mon Nov 19 11:13:29 PST 2012
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.
Cheers
Michael
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/20121119/9b627a78/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parser.patch
Type: application/octet-stream
Size: 9548 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121119/9b627a78/attachment.obj>
More information about the cfe-commits
mailing list