r310449 - [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements

[Nico Weber via cfe-commits]

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 at 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/
> SemaStmt.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/
> switch.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 at -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/warn-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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170809/d018efd8/attachment.html>