[PATCH] Recover from errors in enum definition.
Serge Pavlov
sepavloff at gmail.com
Mon Dec 30 10:22:06 PST 2013
Thank you for review, Alp!
2013/12/30 Alp Toker <alp at nuanti.com>
>
> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
--
Thanks,
--Serge
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131231/a650cefc/attachment.html>
More information about the cfe-commits
mailing list