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