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