Can we move this behind a Wenum-compare subgroup, say Wenum-compare-type? Our codebase is (well, was) Wenum-compare clean, and this new warnings fires pretty often and from a first quick glance the warning looks pretty low-signal anyways (*). Maybe the subgroup shouldn't even be on by default -- do you have any data on true / false positive rate of this?
(But for starters, just having a way to turn this off is enough.) For example, we have a windows common control that's either a PGRP_DOWN or a PGRP_UP page control and depending on which you store the control state in the same int, then stuff like `return extra.inner_spin.spin_up ? UPS_DISABLED : DNS_DISABLED;`. On Mon, Sep 30, 2019 at 3:53 PM David Bolvansky via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: xbolva00 > Date: Mon Sep 30 12:55:50 2019 > New Revision: 373252 > > URL: http://llvm.org/viewvc/llvm-project?rev=373252&view=rev > Log: > [Diagnostics] Warn if enumeration type mismatch in conditional expression > > Summary: > - Useful warning > - GCC compatibility (GCC warns in C++ mode) > > Reviewers: rsmith, aaron.ballman > > Reviewed By: aaron.ballman > > Subscribers: cfe-commits > > Tags: #clang > > Differential Revision: https://reviews.llvm.org/D67919 > > Added: > cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c > Modified: > cfe/trunk/lib/Sema/SemaChecking.cpp > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=373252&r1=373251&r2=373252&view=diff > > ============================================================================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Sep 30 12:55:50 2019 > @@ -11308,6 +11308,32 @@ static const IntegerLiteral *getIntegerL > return IL; > } > > +static void CheckConditionalWithEnumTypes(Sema &S, SourceLocation Loc, > + Expr *LHS, Expr *RHS) { > + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType(); > + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType(); > + > + const auto *LHSEnumType = LHSStrippedType->getAs<EnumType>(); > + if (!LHSEnumType) > + return; > + const auto *RHSEnumType = RHSStrippedType->getAs<EnumType>(); > + if (!RHSEnumType) > + return; > + > + // Ignore anonymous enums. > + if (!LHSEnumType->getDecl()->hasNameForLinkage()) > + return; > + if (!RHSEnumType->getDecl()->hasNameForLinkage()) > + return; > + > + if (S.Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType)) > + return; > + > + S.Diag(Loc, diag::warn_conditional_mixed_enum_types) > + << LHSStrippedType << RHSStrippedType << LHS->getSourceRange() > + << RHS->getSourceRange(); > +} > + > static void DiagnoseIntInBoolContext(Sema &S, Expr *E) { > E = E->IgnoreParenImpCasts(); > SourceLocation ExprLoc = E->getExprLoc(); > @@ -11799,6 +11825,8 @@ static void CheckConditionalOperator(Sem > bool Suspicious = false; > CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious); > CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious); > + CheckConditionalWithEnumTypes(S, E->getBeginLoc(), E->getTrueExpr(), > + E->getFalseExpr()); > > if (T->isBooleanType()) > DiagnoseIntInBoolContext(S, E); > > Added: cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c?rev=373252&view=auto > > ============================================================================== > --- cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c (added) > +++ cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c Mon Sep 30 > 12:55:50 2019 > @@ -0,0 +1,39 @@ > +// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare %s > +// RUN: %clang_cc1 -x c -fsyntax-only -verify %s > +// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare %s > +// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s > + > +enum ro { A = 0x10 }; > +enum rw { B = 0xFF }; > +enum { C = 0x1A}; > + > +enum { > + STATUS_SUCCESS, > + STATUS_FAILURE, > + MAX_BASE_STATUS_CODE > +}; > + > +enum ExtendedStatusCodes { > + STATUS_SOMETHING_INTERESTING = MAX_BASE_STATUS_CODE + 1000, > +}; > + > + > +int get_flag(int cond) { > + return cond ? A : B; > + #ifdef __cplusplus > + // expected-warning@-2 {{enumeration type mismatch in conditional > expression ('ro' and 'rw')}} > + #else > + // expected-no-diagnostics > + #endif > +} > + > +// In the following cases we purposefully differ from GCC and dont warn > because > +// this code pattern is quite sensitive and we dont want to produce so > many false positives. > + > +int get_flag_anon_enum(int cond) { > + return cond ? A : C; > +} > + > +int foo(int c) { > + return c ? STATUS_SOMETHING_INTERESTING : STATUS_SUCCESS; > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
