r373252 - [Diagnostics] Warn if enumeration type mismatch in conditional expression

[Nico Weber via cfe-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191001/a6b82f46/attachment-0001.html>