Thank you for review, Alp!
2013/12/30 Alp Toker <[email protected]> > > On 30/12/2013 03:45, Serge Pavlov wrote: > >> Updated the patch for new interface of SkipUntil. >> > > Hi Serge, > > You've had this patch for a while so I'll try to get the ball rolling. > > There's some trailing whitespace and formatting which you can fix with > clang-format but I'll focus on the implementation from here out.. Ops. Thank you for noticing that. > > > >> http://llvm-reviews.chandlerc.com/D2116 >> >> CHANGE SINCE LAST DIFF >> http://llvm-reviews.chandlerc.com/D2116?vs=5391&id=6309#toc >> >> Files: >> include/clang/Basic/DiagnosticCommonKinds.td >> lib/Parse/ParseDecl.cpp >> test/Parser/cxx0x-ambig.cpp >> test/Parser/declarators.c >> >> >> > D2116.2.patch >> >> >> Index: include/clang/Basic/DiagnosticCommonKinds.td >> =================================================================== >> --- include/clang/Basic/DiagnosticCommonKinds.td >> +++ include/clang/Basic/DiagnosticCommonKinds.td >> @@ -62,6 +62,7 @@ >> def err_expected : Error<"expected %0">; >> def err_expected_either : Error<"expected %0 or %1">; >> +def err_expected_one_of_three : Error<"expected %0 or %1 or %2">; >> def err_expected_after : Error<"expected %1 after %0">; >> def err_param_redefinition : Error<"redefinition of parameter %0">; >> >> Index: lib/Parse/ParseDecl.cpp >> =================================================================== >> --- lib/Parse/ParseDecl.cpp >> +++ lib/Parse/ParseDecl.cpp >> @@ -3888,58 +3888,82 @@ >> >> Decl *LastEnumConstDecl = 0; >> // Parse the enumerator-list. >> - while (Tok.is(tok::identifier)) { >> - IdentifierInfo *Ident = Tok.getIdentifierInfo(); >> - SourceLocation IdentLoc = ConsumeToken(); >> - >> - // If attributes exist after the enumerator, parse them. >> - ParsedAttributesWithRange attrs(AttrFactory); >> - MaybeParseGNUAttributes(attrs); >> - MaybeParseCXX11Attributes(attrs); >> - ProhibitAttributes(attrs); >> - >> - SourceLocation EqualLoc; >> - ExprResult AssignedVal; >> - ParsingDeclRAIIObject PD(*this, ParsingDeclRAIIObject::NoParent); >> - >> - if (TryConsumeToken(tok::equal, EqualLoc)) { >> - AssignedVal = ParseConstantExpression(); >> - if (AssignedVal.isInvalid()) >> - SkipUntil(tok::comma, tok::r_brace, StopAtSemi | >> StopBeforeMatch); >> - } >> - >> - // Install the enumerator constant into EnumDecl. >> - Decl *EnumConstDecl = Actions.ActOnEnumConstant(getCurScope(), >> EnumDecl, >> - LastEnumConstDecl, >> - IdentLoc, Ident, >> - attrs.getList(), >> EqualLoc, >> - >> AssignedVal.release()); >> - PD.complete(EnumConstDecl); >> - >> - EnumConstantDecls.push_back(EnumConstDecl); >> - LastEnumConstDecl = EnumConstDecl; >> - >> - if (Tok.is(tok::identifier)) { >> - // We're missing a comma between enumerators. >> - SourceLocation Loc = PP.getLocForEndOfToken(PrevTokLocation); >> - Diag(Loc, diag::err_enumerator_list_missing_comma) >> - << FixItHint::CreateInsertion(Loc, ", "); >> - continue; >> - } >> - >> - SourceLocation CommaLoc; >> - if (!TryConsumeToken(tok::comma, CommaLoc)) >> - break; >> - >> - if (Tok.isNot(tok::identifier)) { >> - if (!getLangOpts().C99 && !getLangOpts().CPlusPlus11) >> - Diag(CommaLoc, getLangOpts().CPlusPlus ? >> - diag::ext_enumerator_list_comma_cxx : >> - diag::ext_enumerator_list_comma_c) >> - << FixItHint::CreateRemoval(CommaLoc); >> - else if (getLangOpts().CPlusPlus11) >> - Diag(CommaLoc, diag::warn_cxx98_compat_enumerator_list_comma) >> - << FixItHint::CreateRemoval(CommaLoc); >> + if (Tok.isNot(tok::r_brace)) { >> > > Nesting is getting deep here. Can this loop be restructured so it's easier > to follow the flow as it iterates through the enumerator-list? > I rearranged conditions and removed one level of nesting, also added some comments. Hope it helps to follow the flow. > > + while (true) { >> + if (Tok.isNot(tok::identifier)) { >> + Diag(Tok.getLocation(), diag::err_expected) << tok::identifier; >> + if (SkipUntil(tok::comma, tok::r_brace, StopBeforeMatch) && >> + TryConsumeToken(tok::comma)) continue; >> + break; >> + } >> >> + IdentifierInfo *Ident = Tok.getIdentifierInfo(); >> + SourceLocation IdentLoc = ConsumeToken(); >> + >> + // If attributes exist after the enumerator, parse them. >> + ParsedAttributesWithRange attrs(AttrFactory); >> + MaybeParseGNUAttributes(attrs); >> + MaybeParseCXX11Attributes(attrs); >> + ProhibitAttributes(attrs); >> + >> + SourceLocation EqualLoc; >> + ExprResult AssignedVal; >> + ParsingDeclRAIIObject PD(*this, ParsingDeclRAIIObject::NoParent); >> + >> + if (TryConsumeToken(tok::equal, EqualLoc)) { >> + AssignedVal = ParseConstantExpression(); >> + if (AssignedVal.isInvalid()) >> + SkipUntil(tok::comma, tok::r_brace, StopBeforeMatch); >> + } >> + >> + // Install the enumerator constant into EnumDecl. >> + Decl *EnumConstDecl = Actions.ActOnEnumConstant(getCurScope(), >> EnumDecl, >> + LastEnumConstDecl, >> + IdentLoc, Ident, >> + attrs.getList(), >> EqualLoc, >> + >> AssignedVal.release()); >> + PD.complete(EnumConstDecl); >> + >> + EnumConstantDecls.push_back(EnumConstDecl); >> + LastEnumConstDecl = EnumConstDecl; >> + >> + if (Tok.is(tok::identifier)) { >> + // We're missing a comma between enumerators. >> + SourceLocation Loc = PP.getLocForEndOfToken(PrevTokLocation); >> + Diag(Loc, diag::err_enumerator_list_missing_comma) >> + << FixItHint::CreateInsertion(Loc, ", "); >> + continue; >> + } >> + >> + if (Tok.is(tok::r_brace)) >> + break; >> + >> + SourceLocation CommaLoc; >> + if (!TryConsumeToken(tok::comma, CommaLoc)) { >> + if (EqualLoc.isValid()) >> + Diag(Tok.getLocation(), diag::err_expected_either) >> + << tok::r_brace << tok::comma; >> + else >> + Diag(Tok.getLocation(), diag::err_expected_one_of_three) >> + << tok::r_brace << tok::comma << tok::equal; >> > > Good work updating your patch to use the new diagnostic formatter. > > Listing three kinds of tokens the parser wants here seems a little > brusque. In this instance it seems better to provide a custom diagnostic > tailored for enumerators. > Tried to reformulate the message text. Probably it is more urbane now. > > > + if (SkipUntil(tok::comma, tok::r_brace, StopBeforeMatch)) { >> + if (TryConsumeToken(tok::comma, CommaLoc)) >> + continue; >> + } else { >> + break; >> + } >> + } >> + >> + if (Tok.is(tok::r_brace)) { >> + if (!getLangOpts().C99 && !getLangOpts().CPlusPlus11) >> + Diag(CommaLoc, getLangOpts().CPlusPlus ? >> + diag::ext_enumerator_list_comma_cxx : >> + diag::ext_enumerator_list_comma_c) >> + << FixItHint::CreateRemoval(CommaLoc); >> + else if (getLangOpts().CPlusPlus11) >> + Diag(CommaLoc, diag::warn_cxx98_compat_enumerator_list_comma) >> + << FixItHint::CreateRemoval(CommaLoc); >> + break; >> + } >> >> } >> } >> Index: test/Parser/cxx0x-ambig.cpp >> =================================================================== >> --- test/Parser/cxx0x-ambig.cpp >> +++ test/Parser/cxx0x-ambig.cpp >> @@ -48,7 +48,7 @@ >> }; >> // This could be a bit-field. >> struct S2 { >> - enum E : T { a = 1, b = 2, c = 3, 4 }; // expected-error >> {{non-integral type}} expected-error {{expected '}'}} expected-note {{to >> match}} >> + enum E : T { a = 1, b = 2, c = 3, 4 }; // expected-error >> {{non-integral type}} expected-error {{expected identifier}} >> }; >> struct S3 { >> enum E : int { a = 1, b = 2, c = 3, d }; // ok, defines an enum >> @@ -64,7 +64,7 @@ >> }; >> // This could be a bit-field. >> struct S6 { >> - enum E : int { 1 }; // expected-error {{expected '}'}} expected-note >> {{to match}} >> + enum E : int { 1 }; // expected-error {{expected identifier}} >> > > Nice QOI improvement here. > > Alp. > > > }; >> struct U { >> Index: test/Parser/declarators.c >> =================================================================== >> --- test/Parser/declarators.c >> +++ test/Parser/declarators.c >> @@ -113,3 +113,37 @@ >> >> struct S { int n; }: // expected-error {{expected ';'}} >> }; >> + >> +// PR10982 >> +enum E11 { >> + A1 = 1, >> +}; >> + >> +enum E12 { >> + , // expected-error{{expected identifier}} >> + A2 >> +}; >> +void func_E12(enum E12 *p) { *p = A2; } >> + >> +enum E13 { >> + 1D, // expected-error{{expected identifier}} >> + A3 >> +}; >> +void func_E13(enum E13 *p) { *p = A3; } >> + >> +enum E14 { >> + A4 12, // expected-error{{expected '}' or ',' or '='}} >> + A4a >> +}; >> +void func_E14(enum E14 *p) { *p = A4a; } >> + >> +enum E15 { >> + A5=12 4, // expected-error{{expected '}' or ','}} >> + A5a >> +}; >> +void func_E15(enum E15 *p) { *p = A5a; } >> + >> +enum E16 { >> + A6; // expected-error{{expected '}' or ',' or '='}} >> + A6a >> +}; >> > > > > _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > -- > http://www.nuanti.com > the browser experts > > -- Thanks, --Serge
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
