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

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 9 07:13:04 PDT 2017


On Wed, Aug 9, 2017 at 10:07 AM, Nico Weber via cfe-commits
<cfe-commits at lists.llvm.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.

I would not be opposed to that.

~Aaron

>
> 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
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


More information about the cfe-commits mailing list