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

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

2017-08-09 Thread Nico Weber via cfe-commits

On Wed, Aug 9, 2017 at 10:11 AM, Gábor Horváth  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  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=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=310448=310449=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>>
>>>  /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
>>>  /// potentially integral-promoted expression @p expr.
>>> -static QualType GetTypeBeforeIntegralPromotion(Expr *) {
>>> -  if (ExprWithCleanups *cleanups = dyn_cast(expr))
>>> -expr = cleanups->getSubExpr();
>>> -  while (ImplicitCastExpr *impcast = dyn_cast(expr))
>>> {
>>> -if (impcast->getCastKind() != CK_IntegralCast) break;
>>> -expr = impcast->getSubExpr();
>>> +static QualType GetTypeBeforeIntegralPromotion(const Expr *) {
>>> +  if (const auto *CleanUps = dyn_cast(E))
>>> +E = CleanUps->getSubExpr();
>>> +  while (const auto *ImpCast = dyn_cast(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 , const Expr *Cond,
>>> +   const Expr *Case) {
>>> +  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
>>> +  QualType CaseType = Case->getType();
>>> +
>>> +  const EnumType *CondEnumType = CondType->getAs();
>>> +  const EnumType *CaseEnumType = CaseType->getAs();
>>> +  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=310448=310449=diff
>>> 
>>> ==
>>> ---

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

2017-08-09 Thread Aaron Ballman via cfe-commits

On Wed, Aug 9, 2017 at 10:07 AM, Nico Weber via cfe-commits
 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
>  wrote:
>>
>> Author: xazax
>> Date: Wed Aug  9 01:57:09 2017
>> New Revision: 310449
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310449=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=310448=310449=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>
>>  /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
>>  /// potentially integral-promoted expression @p expr.
>> -static QualType GetTypeBeforeIntegralPromotion(Expr *) {
>> -  if (ExprWithCleanups *cleanups = dyn_cast(expr))
>> -expr = cleanups->getSubExpr();
>> -  while (ImplicitCastExpr *impcast = dyn_cast(expr)) {
>> -if (impcast->getCastKind() != CK_IntegralCast) break;
>> -expr = impcast->getSubExpr();
>> +static QualType GetTypeBeforeIntegralPromotion(const Expr *) {
>> +  if (const auto *CleanUps = dyn_cast(E))
>> +E = CleanUps->getSubExpr();
>> +  while (const auto *ImpCast = dyn_cast(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 , const Expr *Cond,
>> +   const Expr *Case) {
>> +  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
>> +  QualType CaseType = Case->getType();
>> +
>> +  const EnumType *CondEnumType = CondType->getAs();
>> +  const EnumType *CaseEnumType = CaseType->getAs();
>> +  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=310448=310449=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/warn-enum-compare.cpp?rev=310449=310448=310449=diff
>>
>> ==
>> --- cfe/trunk/test/SemaCXX/warn-enum-compare.cpp (original)
>>

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

2017-08-09 Thread Gábor Horváth via cfe-commits

Sure!

There was a follow-up patch that excluded anonymous enums. Is it still that
bad to introduce it under a new flag?

Regards,
Gábor

On 9 August 2017 at 16:07, Nico Weber  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=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=310448=310449=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>
>>  /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
>>  /// potentially integral-promoted expression @p expr.
>> -static QualType GetTypeBeforeIntegralPromotion(Expr *) {
>> -  if (ExprWithCleanups *cleanups = dyn_cast(expr))
>> -expr = cleanups->getSubExpr();
>> -  while (ImplicitCastExpr *impcast = dyn_cast(expr)) {
>> -if (impcast->getCastKind() != CK_IntegralCast) break;
>> -expr = impcast->getSubExpr();
>> +static QualType GetTypeBeforeIntegralPromotion(const Expr *) {
>> +  if (const auto *CleanUps = dyn_cast(E))
>> +E = CleanUps->getSubExpr();
>> +  while (const auto *ImpCast = dyn_cast(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 , const Expr *Cond,
>> +   const Expr *Case) {
>> +  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
>> +  QualType CaseType = Case->getType();
>> +
>> +  const EnumType *CondEnumType = CondType->getAs();
>> +  const EnumType *CaseEnumType = CaseType->getAs();
>> +  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=310448=310449=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/
>> warn-enum-compare.cpp?rev=310449=310448=310449=diff
>> 
>>

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

2017-08-09 Thread 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@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=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=310448=310449=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
>  /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
>  /// potentially integral-promoted expression @p expr.
> -static QualType GetTypeBeforeIntegralPromotion(Expr *) {
> -  if (ExprWithCleanups *cleanups = dyn_cast(expr))
> -expr = cleanups->getSubExpr();
> -  while (ImplicitCastExpr *impcast = dyn_cast(expr)) {
> -if (impcast->getCastKind() != CK_IntegralCast) break;
> -expr = impcast->getSubExpr();
> +static QualType GetTypeBeforeIntegralPromotion(const Expr *) {
> +  if (const auto *CleanUps = dyn_cast(E))
> +E = CleanUps->getSubExpr();
> +  while (const auto *ImpCast = dyn_cast(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 , const Expr *Cond,
> +   const Expr *Case) {
> +  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
> +  QualType CaseType = Case->getType();
> +
> +  const EnumType *CondEnumType = CondType->getAs();
> +  const EnumType *CaseEnumType = CaseType->getAs();
> +  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=310448=310449=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/warn-enum-compare.cpp?rev=310449=310448=310449=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);

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

2017-08-09 Thread Gabor Horvath via cfe-commits

Author: xazax
Date: Wed Aug  9 01:57:09 2017
New Revision: 310449

URL: http://llvm.org/viewvc/llvm-project?rev=310449=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=310448=310449=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(expr))
-expr = cleanups->getSubExpr();
-  while (ImplicitCastExpr *impcast = dyn_cast(expr)) {
-if (impcast->getCastKind() != CK_IntegralCast) break;
-expr = impcast->getSubExpr();
+static QualType GetTypeBeforeIntegralPromotion(const Expr *) {
+  if (const auto *CleanUps = dyn_cast(E))
+E = CleanUps->getSubExpr();
+  while (const auto *ImpCast = dyn_cast(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 , const Expr *Cond,
+   const Expr *Case) {
+  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
+  QualType CaseType = Case->getType();
+
+  const EnumType *CondEnumType = CondType->getAs();
+  const EnumType *CaseEnumType = CaseType->getAs();
+  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=310448=310449=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/warn-enum-compare.cpp?rev=310449=310448=310449=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

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information