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

[Nico Weber via cfe-commits]

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/345af0bb/attachment.html>