On Wed, Aug 9, 2017 at 10:11 AM, Gábor Horváth <xazax....@gmail.com> wrote:
> Sure! > > There was a follow-up patch that excluded anonymous enums. Is it still > that bad to introduce it under a new flag? > The follow-up was at r310468. https://build.chromium.org/p/chromium.fyi/ builders/ClangToTLinux/builds/8808 is with clang 310476 (i.e. it has the follow-up) and things still don't build ( https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FClangToTLinux%2F8808%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout). The warning are in an external dependency (protobuf) that we can't change very quickly. (And I'd imagine that other codebases out there will have the same problem.) So yes, a different flag would still be good. > > Regards, > Gábor > > > On 9 August 2017 at 16:07, Nico Weber <tha...@chromium.org> wrote: > >> 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. >> >> On Wed, Aug 9, 2017 at 4:57 AM, Gabor Horvath via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: xazax >>> Date: Wed Aug 9 01:57:09 2017 >>> New Revision: 310449 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=310449&view=rev >>> Log: >>> [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch >>> statements >>> >>> Patch by: Reka Nikolett Kovacs >>> >>> Differential Revision: https://reviews.llvm.org/D36407 >>> >>> Modified: >>> cfe/trunk/lib/Sema/SemaStmt.cpp >>> cfe/trunk/test/Sema/switch.c >>> cfe/trunk/test/SemaCXX/warn-enum-compare.cpp >>> >>> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaS >>> tmt.cpp?rev=310449&r1=310448&r2=310449&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original) >>> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Wed Aug 9 01:57:09 2017 >>> @@ -602,14 +602,14 @@ static bool EqEnumVals(const std::pair<l >>> >>> /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of >>> /// potentially integral-promoted expression @p expr. >>> -static QualType GetTypeBeforeIntegralPromotion(Expr *&expr) { >>> - if (ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(expr)) >>> - expr = cleanups->getSubExpr(); >>> - while (ImplicitCastExpr *impcast = dyn_cast<ImplicitCastExpr>(expr)) >>> { >>> - if (impcast->getCastKind() != CK_IntegralCast) break; >>> - expr = impcast->getSubExpr(); >>> +static QualType GetTypeBeforeIntegralPromotion(const Expr *&E) { >>> + if (const auto *CleanUps = dyn_cast<ExprWithCleanups>(E)) >>> + E = CleanUps->getSubExpr(); >>> + while (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) { >>> + if (ImpCast->getCastKind() != CK_IntegralCast) break; >>> + E = ImpCast->getSubExpr(); >>> } >>> - return expr->getType(); >>> + return E->getType(); >>> } >>> >>> ExprResult Sema::CheckSwitchCondition(SourceLocation SwitchLoc, Expr >>> *Cond) { >>> @@ -743,6 +743,24 @@ static bool ShouldDiagnoseSwitchCaseNotI >>> return true; >>> } >>> >>> +static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond, >>> + const Expr *Case) { >>> + QualType CondType = GetTypeBeforeIntegralPromotion(Cond); >>> + QualType CaseType = Case->getType(); >>> + >>> + const EnumType *CondEnumType = CondType->getAs<EnumType>(); >>> + const EnumType *CaseEnumType = CaseType->getAs<EnumType>(); >>> + if (!CondEnumType || !CaseEnumType) >>> + return; >>> + >>> + if (S.Context.hasSameUnqualifiedType(CondType, CaseType)) >>> + return; >>> + >>> + S.Diag(Case->getExprLoc(), diag::warn_comparison_of_mixed_enum_types) >>> + << CondType << CaseType << Cond->getSourceRange() >>> + << Case->getSourceRange(); >>> +} >>> + >>> StmtResult >>> Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch, >>> Stmt *BodyStmt) { >>> @@ -760,7 +778,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocati >>> >>> QualType CondType = CondExpr->getType(); >>> >>> - Expr *CondExprBeforePromotion = CondExpr; >>> + const Expr *CondExprBeforePromotion = CondExpr; >>> QualType CondTypeBeforePromotion = >>> GetTypeBeforeIntegralPromotion(CondExprBeforePromotion); >>> >>> @@ -843,6 +861,8 @@ Sema::ActOnFinishSwitchStmt(SourceLocati >>> break; >>> } >>> >>> + checkEnumTypesInSwitchStmt(*this, CondExpr, Lo); >>> + >>> llvm::APSInt LoVal; >>> >>> if (getLangOpts().CPlusPlus11) { >>> >>> Modified: cfe/trunk/test/Sema/switch.c >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/swit >>> ch.c?rev=310449&r1=310448&r2=310449&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/Sema/switch.c (original) >>> +++ cfe/trunk/test/Sema/switch.c Wed Aug 9 01:57:09 2017 >>> @@ -372,6 +372,7 @@ void switch_on_ExtendedEnum1(enum Extend >>> case EE1_b: break; >>> case EE1_c: break; // no-warning >>> case EE1_d: break; // expected-warning {{case value not in enumerated >>> type 'enum ExtendedEnum1'}} >>> + // expected-warning@-1 {{comparison of two values with different >>> enumeration types ('enum ExtendedEnum1' and 'const enum >>> ExtendedEnum1_unrelated')}} >>> } >>> } >>> >>> >>> Modified: cfe/trunk/test/SemaCXX/warn-enum-compare.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/w >>> arn-enum-compare.cpp?rev=310449&r1=310448&r2=310449&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/SemaCXX/warn-enum-compare.cpp (original) >>> +++ cfe/trunk/test/SemaCXX/warn-enum-compare.cpp Wed Aug 9 01:57:09 >>> 2017 >>> @@ -209,4 +209,21 @@ void test () { >>> while (getBar() > x); // expected-warning {{comparison of two values >>> with different enumeration types ('Bar' and 'Foo')}} >>> while (getBar() < x); // expected-warning {{comparison of two values >>> with different enumeration types ('Bar' and 'Foo')}} >>> >>> + switch (a) { >>> + case name1::F1: break; >>> + case name1::F3: break; >>> + case name2::B2: break; // expected-warning {{comparison of two >>> values with different enumeration types ('name1::Foo' and 'name2::Baz')}} >>> + } >>> + >>> + switch (x) { >>> + case FooB: break; >>> + case FooC: break; >>> + case BarD: break; // expected-warning {{comparison of two values >>> with different enumeration types ('Foo' and 'Bar')}} >>> + } >>> + >>> + switch(getBar()) { >>> + case BarE: break; >>> + case BarF: break; >>> + case FooA: break; // expected-warning {{comparison of two values >>> with different enumeration types ('Bar' and 'Foo')}} >>> + } >>> } >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits