Done. rL373371.
ut 1. 10. 2019 o 19:58 Dávid Bolvanský <david.bolvan...@gmail.com> napísal(a): > > 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