Sorry, answered on phone, missed it. I looked at your buildbots for chromium and yeah, so many warnings. I will make it off by default.
ut 1. 10. 2019 o 19:58 Dávid Bolvanský <david.bolvan...@gmail.com> napísal(a): > > (Not sure if you intentionally didn't reply-all.) > > Sorry, answered on phone, missed it. I looked at your buildbots for > chromium and yeah, so many warnings. I will make it off by default. > > ut 1. 10. 2019 o 19:17 Nico Weber <tha...@chromium.org> napísal(a): > > > > > (Not sure if you intentionally didn't reply-all.) > > > > We should probably write it down, but clang's warning philosophy is that > > warnings should be very high-signal. The usual way to prove this is to > > build some large open-source codebase with the warning and counting true > > and false positives. > > > > Just "gcc has it" isn't sufficient motivation – gcc has lots of not so good > > warnings :) > > > > ps: To be clear, I appreciate all your work to add warnings a lot! It's > > very useful. It's also possible that this warning is useful, but it's not > > immediately clear, so there should be some data to back it up. > > > > On Tue, Oct 1, 2019 at 12:12 PM Dávid Bolvanský <david.bolvan...@gmail.com> > > wrote: > >> > >> I hit this bug myself, GCC warned me with -Wenum-compare, Clang was > >> silent. Clang “supports” this flag, but failed to catch it. Either Clang > >> should not pretend that it supports this flag (and all related > >> functionality) or should support it fully. Basically, new warnings will > >> show up only for Clang-only environments. > >> > >> I think the own subgroup is a good compromise for now. We can still make > >> it off by default before next release if we decide so. > >> > >> Dňa 1. 10. 2019 o 17:56 užívateľ Nico Weber <tha...@chromium.org> napísal: > >> > >> > >> 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
