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 <[email protected]>wrote: > On Mon, Nov 19, 2012 at 11:13 AM, Michael Han <[email protected]> > 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 <[email protected]> > > 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 <[email protected]> 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 < > [email protected]> > >> > 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 < > [email protected]> > >> >> wrote: > >> >>> > >> >>> On Thu, Nov 15, 2012 at 10:33 AM, Michael Han > >> >>> <[email protected]> > >> >>> 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 > >> >> [email protected] > >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> >> > > > > >
update.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
