r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
Dávid Bolvanský via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 1 11:10:17 PDT 2019
Done. rL373371.
ut 1. 10. 2019 o 19:58 Dávid Bolvanský <david.bolvansky at 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.bolvansky at 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 <thakis at 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.bolvansky at 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 <thakis at 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.bolvansky at gmail.com> wrote:
> > >>>
> > >>> Yeah, I moved this warning into own subgroup in rL373345. Thanks.
> > >>>
> > >>> ut 1. 10. 2019 o 16:24 Nico Weber <thakis at 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 at 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 at -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 at lists.llvm.org
> > >>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list