r310449 - [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 9 08:35:22 PDT 2017
On Wed, Aug 9, 2017 at 10:11 AM, Gábor Horváth <xazax.hun at 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 <thakis at 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 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/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 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/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 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/9fb6301c/attachment-0001.html>
More information about the cfe-commits
mailing list