<div dir="ltr"><div><div><div>Sure!<br><br></div>There was a follow-up patch that excluded anonymous enums. Is it still that bad to introduce it under a new flag?<br><br></div>Regards,<br></div>Gábor<br><div><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On 9 August 2017 at 16:07, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Any way we could put this behind a different flag (say, -Wenum-compare-switch, in the same group as -Wenum-compare, so that -W(no-)enum-compare affects both)? Our codebase was -Wenum-compare clean before this commit but is very not clean now, so we'd now have to disable -Wenum-compare altogether, but then we won't catch regressions in non-switch statements either.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 9, 2017 at 4:57 AM, Gabor Horvath via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: xazax<br>
Date: Wed Aug 9 01:57:09 2017<br>
New Revision: 310449<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=310449&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=310449&view=rev</a><br>
Log:<br>
[Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements<br>
<br>
Patch by: Reka Nikolett Kovacs<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D36407" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3640<wbr>7</a><br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaStmt.cp<wbr>p<br>
cfe/trunk/test/Sema/switch.c<br>
cfe/trunk/test/SemaCXX/warn-en<wbr>um-compare.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaStmt.cp<wbr>p<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=310449&r1=310448&r2=310449&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/Sema/SemaS<wbr>tmt.cpp?rev=310449&r1=310448&<wbr>r2=310449&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaStmt.cp<wbr>p (original)<br>
+++ cfe/trunk/lib/Sema/SemaStmt.cp<wbr>p Wed Aug 9 01:57:09 2017<br>
@@ -602,14 +602,14 @@ static bool EqEnumVals(const std::pair<l<br>
<br>
/// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of<br>
/// potentially integral-promoted expression @p expr.<br>
-static QualType GetTypeBeforeIntegralPromotion<wbr>(Expr *&expr) {<br>
- if (ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(exp<wbr>r))<br>
- expr = cleanups->getSubExpr();<br>
- while (ImplicitCastExpr *impcast = dyn_cast<ImplicitCastExpr>(exp<wbr>r)) {<br>
- if (impcast->getCastKind() != CK_IntegralCast) break;<br>
- expr = impcast->getSubExpr();<br>
+static QualType GetTypeBeforeIntegralPromotion<wbr>(const Expr *&E) {<br>
+ if (const auto *CleanUps = dyn_cast<ExprWithCleanups>(E))<br>
+ E = CleanUps->getSubExpr();<br>
+ while (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {<br>
+ if (ImpCast->getCastKind() != CK_IntegralCast) break;<br>
+ E = ImpCast->getSubExpr();<br>
}<br>
- return expr->getType();<br>
+ return E->getType();<br>
}<br>
<br>
ExprResult Sema::CheckSwitchCondition(Sou<wbr>rceLocation SwitchLoc, Expr *Cond) {<br>
@@ -743,6 +743,24 @@ static bool ShouldDiagnoseSwitchCaseNotI<br>
return true;<br>
}<br>
<br>
+static void checkEnumTypesInSwitchStmt(Sem<wbr>a &S, const Expr *Cond,<br>
+ const Expr *Case) {<br>
+ QualType CondType = GetTypeBeforeIntegralPromotion<wbr>(Cond);<br>
+ QualType CaseType = Case->getType();<br>
+<br>
+ const EnumType *CondEnumType = CondType->getAs<EnumType>();<br>
+ const EnumType *CaseEnumType = CaseType->getAs<EnumType>();<br>
+ if (!CondEnumType || !CaseEnumType)<br>
+ return;<br>
+<br>
+ if (S.Context.hasSameUnqualifiedT<wbr>ype(CondType, CaseType))<br>
+ return;<br>
+<br>
+ S.Diag(Case->getExprLoc(), diag::warn_comparison_of_mixed<wbr>_enum_types)<br>
+ << CondType << CaseType << Cond->getSourceRange()<br>
+ << Case->getSourceRange();<br>
+}<br>
+<br>
StmtResult<br>
Sema::ActOnFinishSwitchStmt(S<wbr>ourceLocation SwitchLoc, Stmt *Switch,<br>
Stmt *BodyStmt) {<br>
@@ -760,7 +778,7 @@ Sema::ActOnFinishSwitchStmt(So<wbr>urceLocati<br>
<br>
QualType CondType = CondExpr->getType();<br>
<br>
- Expr *CondExprBeforePromotion = CondExpr;<br>
+ const Expr *CondExprBeforePromotion = CondExpr;<br>
QualType CondTypeBeforePromotion =<br>
<wbr>GetTypeBeforeIntegralPromotion<wbr>(CondExprBeforePromotion);<br>
<br>
@@ -843,6 +861,8 @@ Sema::ActOnFinishSwitchStmt(So<wbr>urceLocati<br>
break;<br>
}<br>
<br>
+ checkEnumTypesInSwitchStmt(*th<wbr>is, CondExpr, Lo);<br>
+<br>
llvm::APSInt LoVal;<br>
<br>
if (getLangOpts().CPlusPlus11) {<br>
<br>
Modified: cfe/trunk/test/Sema/switch.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/switch.c?rev=310449&r1=310448&r2=310449&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Sema/swit<wbr>ch.c?rev=310449&r1=310448&r2=<wbr>310449&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Sema/switch.c (original)<br>
+++ cfe/trunk/test/Sema/switch.c Wed Aug 9 01:57:09 2017<br>
@@ -372,6 +372,7 @@ void switch_on_ExtendedEnum1(enum Extend<br>
case EE1_b: break;<br>
case EE1_c: break; // no-warning<br>
case EE1_d: break; // expected-warning {{case value not in enumerated type 'enum ExtendedEnum1'}}<br>
+ // expected-warning@-1 {{comparison of two values with different enumeration types ('enum ExtendedEnum1' and 'const enum ExtendedEnum1_unrelated')}}<br>
}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/test/SemaCXX/warn-en<wbr>um-compare.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-enum-compare.cpp?rev=310449&r1=310448&r2=310449&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/SemaCXX/<wbr>warn-enum-compare.cpp?rev=<wbr>310449&r1=310448&r2=310449&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/SemaCXX/warn-en<wbr>um-compare.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/warn-en<wbr>um-compare.cpp Wed Aug 9 01:57:09 2017<br>
@@ -209,4 +209,21 @@ void test () {<br>
while (getBar() > x); // expected-warning {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}<br>
while (getBar() < x); // expected-warning {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}<br>
<br>
+ switch (a) {<br>
+ case name1::F1: break;<br>
+ case name1::F3: break;<br>
+ case name2::B2: break; // expected-warning {{comparison of two values with different enumeration types ('name1::Foo' and 'name2::Baz')}}<br>
+ }<br>
+<br>
+ switch (x) {<br>
+ case FooB: break;<br>
+ case FooC: break;<br>
+ case BarD: break; // expected-warning {{comparison of two values with different enumeration types ('Foo' and 'Bar')}}<br>
+ }<br>
+<br>
+ switch(getBar()) {<br>
+ case BarE: break;<br>
+ case BarF: break;<br>
+ case FooA: break; // expected-warning {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}<br>
+ }<br>
}<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></div></div></div>