Hi Richard, Is the updated patch ok to commit?
Cheers Michael On Tue, Nov 20, 2012 at 7:10 PM, Michael Han <[email protected]>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 <[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 >> >> >> >> > >> > >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
