Thanks! Any thoughts on the true positive rate for this warning?
On Tue, Oct 1, 2019 at 11:43 AM Dávid Bolvanský <david.bolvan...@gmail.com> wrote: > Yeah, I moved this warning into own subgroup in rL373345. Thanks. > > ut 1. 10. 2019 o 16:24 Nico Weber <tha...@chromium.org> napísal(a): > > > > 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
