Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
Chromium shouldn't be a factor for determining is a warning is on or off by default. We can easily turn warnings off, and I had done so in http://crrev.com/http://crrev.com/701604 (as long as warnings have a dedicated flag -- but that's needed for every large codebase adopting a new clang). So that's not super relevant -- the bots would've cycled green anyways. 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. Another thing we sometimes say that a warning should be so good that it should be possible to turn it on by default, and if a warning doesn't meet that bar, then maybe we shouldn't have it. (There are exceptions.) 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 1:59 PM Dávid Bolvanský wrote: > 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ý > 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 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 > 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 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=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: >
Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
Done. rL373371.
ut 1. 10. 2019 o 19:58 Dávid Bolvanský 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ý 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 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ý
> > > 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
> > >> 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ý
> > >> wrote:
> > >>>
> > >>> Yeah, I moved this warning into own subgroup in rL373345. Thanks.
> > >>>
> > >>> ut 1. 10. 2019 o 16:24 Nico Weber 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
> > >>> > wrote:
> > >>> >>
> > >>> >> Author: xbolva00
> > >>> >> Date: Mon Sep 30 12:55:50 2019
> > >>> >> New Revision: 373252
> > >>> >>
> > >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=373252=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=373251=373252=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 , SourceLocation
> > >>> >> Loc,
> > >>> >> + Expr *LHS, Expr *RHS) {
> > >>> >> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
> > >>> >> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
> > >>> >> +
> > >>> >> + const auto *LHSEnumType = LHSStrippedType->getAs();
> > >>> >> + if (!LHSEnumType)
> > >>> >> +return;
> >
Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
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ý 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 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ý
> > 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 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ý
> >> wrote:
> >>>
> >>> Yeah, I moved this warning into own subgroup in rL373345. Thanks.
> >>>
> >>> ut 1. 10. 2019 o 16:24 Nico Weber 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
> >>> > wrote:
> >>> >>
> >>> >> Author: xbolva00
> >>> >> Date: Mon Sep 30 12:55:50 2019
> >>> >> New Revision: 373252
> >>> >>
> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=373252=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=373251=373252=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 , SourceLocation Loc,
> >>> >> + Expr *LHS, Expr *RHS) {
> >>> >> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
> >>> >> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
> >>> >> +
> >>> >> + const auto *LHSEnumType = LHSStrippedType->getAs();
> >>> >> + if (!LHSEnumType)
> >>> >> +return;
> >>> >> + const auto *RHSEnumType = RHSStrippedType->getAs();
> >>> >> + if (!RHSEnumType)
> >>> >> +return;
> >>> >> +
> >>> >> + // Ignore anonymous enums.
> >>> >> + if (!LHSEnumType->getDecl()->hasNameForLinkage())
> >>> >> +return;
> >>> >> + if (!RHSEnumType->getDecl()->hasNameForLinkage())
> >>> >> +
Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
Thanks!
Any thoughts on the true positive rate for this warning?
On Tue, Oct 1, 2019 at 11:43 AM Dávid Bolvanský
wrote:
> Yeah, I moved this warning into own subgroup in rL373345. Thanks.
>
> ut 1. 10. 2019 o 16:24 Nico Weber 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=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=373251=373252=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 , SourceLocation Loc,
> >> + Expr *LHS, Expr *RHS) {
> >> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
> >> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
> >> +
> >> + const auto *LHSEnumType = LHSStrippedType->getAs();
> >> + if (!LHSEnumType)
> >> +return;
> >> + const auto *RHSEnumType = RHSStrippedType->getAs();
> >> + 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 , 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=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
Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
Yeah, I moved this warning into own subgroup in rL373345. Thanks.
ut 1. 10. 2019 o 16:24 Nico Weber 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
> wrote:
>>
>> Author: xbolva00
>> Date: Mon Sep 30 12:55:50 2019
>> New Revision: 373252
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=373252=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=373251=373252=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 , SourceLocation Loc,
>> + Expr *LHS, Expr *RHS) {
>> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
>> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
>> +
>> + const auto *LHSEnumType = LHSStrippedType->getAs();
>> + if (!LHSEnumType)
>> +return;
>> + const auto *RHSEnumType = RHSStrippedType->getAs();
>> + 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 , 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=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
Re: r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression
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=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=373251=373252=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 , SourceLocation Loc,
> + Expr *LHS, Expr *RHS) {
> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
> +
> + const auto *LHSEnumType = LHSStrippedType->getAs();
> + if (!LHSEnumType)
> +return;
> + const auto *RHSEnumType = RHSStrippedType->getAs();
> + 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 , 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=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
>
