From b0f0a1ea03b55df2c0fcd7e7d56006b7a849124b Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Wed, 20 Sep 2017 09:54:47 +0000 Subject: [PATCH] [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare Recommit. Original commit was reverted because buildbots broke. The error was only reproducible in the build with assertions. The problem was that the diagnostic expected true/false as bool, while it was provided as string "true"/"false". Summary: As requested by Sam McCall: > Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX) > The warning strongly suggests that the enum < 0 check has no effect > (for enums with nonnegative ranges). > Clang doesn't seem to optimize such checks out though, and they seem > likely to catch bugs in some cases. Yes, only if there's UB elsewhere, > but I assume not optimizing out these checks indicates a deliberate > decision to stay somewhat compatible with a technically-incorrect > mental model. > If this is the case, should we move these to a > -Wtautological-compare-enum subcategory? Reviewers: rjmccall, rsmith, aaron.ballman, sammccall, bkramer, djasper Reviewed By: aaron.ballman Subscribers: jroelofs, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D37629 llvm-svn: 313745 --- clang/include/clang/Basic/DiagnosticGroups.td | 2 + clang/include/clang/Basic/DiagnosticSemaKinds.td | 19 +++++--- clang/lib/Sema/SemaChecking.cpp | 38 +++++++++------- clang/test/Sema/compare.c | 53 +++++++++++++++++++++- .../Sema/tautological-unsigned-enum-zero-compare.c | 46 +++++++++++++++++++ 5 files changed, 134 insertions(+), 24 deletions(-) create mode 100644 clang/test/Sema/tautological-unsigned-enum-zero-compare.c diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 33cae6c..9994be4 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -429,12 +429,14 @@ def StringPlusInt : DiagGroup<"string-plus-int">; def StringPlusChar : DiagGroup<"string-plus-char">; def StrncatSize : DiagGroup<"strncat-size">; def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">; +def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">; def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">; def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">; def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">; def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">; def TautologicalCompare : DiagGroup<"tautological-compare", [TautologicalUnsignedZeroCompare, + TautologicalUnsignedEnumZeroCompare, TautologicalOutOfRangeCompare, TautologicalPointerCompare, TautologicalOverlapCompare, diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 16fee891..35d8bef 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -5914,19 +5914,26 @@ def note_typecheck_assign_const : Note< "member function %q1 is declared const here|" "%select{|nested }1data member %2 declared const here}0">; +def warn_lunsigned_always_true_comparison : Warning< + "comparison of unsigned expression %0 is always %select{false|true}1">, + InGroup; +def warn_runsigned_always_true_comparison : Warning< + "comparison of %0 unsigned expression is always %select{false|true}1">, + InGroup; +def warn_lunsigned_enum_always_true_comparison : Warning< + "comparison of unsigned enum expression %0 is always %select{false|true}1">, + InGroup; +def warn_runsigned_enum_always_true_comparison : Warning< + "comparison of %0 unsigned enum expression is always %select{false|true}1">, + InGroup; + def warn_mixed_sign_comparison : Warning< "comparison of integers of different signs: %0 and %1">, InGroup, DefaultIgnore; -def warn_lunsigned_always_true_comparison : Warning< - "comparison of unsigned%select{| enum}2 expression %0 is always %1">, - InGroup; def warn_out_of_range_compare : Warning< "comparison of %select{constant %0|true|false}1 with " "%select{expression of type %2|boolean expression}3 is always " "%select{false|true}4">, InGroup; -def warn_runsigned_always_true_comparison : Warning< - "comparison of %0 unsigned%select{| enum}2 expression is always %1">, - InGroup; def warn_comparison_of_mixed_enum_types : Warning< "comparison of two values with different enumeration types" "%diff{ ($ and $)|}0,1">, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 212fe65..87634ad 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -8583,7 +8583,7 @@ bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) { // bool values are handled by DiagnoseOutOfRangeComparison(). - BinaryOperatorKind op = E->getOpcode(); + BinaryOperatorKind Op = E->getOpcode(); if (E->isValueDependent()) return false; @@ -8592,22 +8592,26 @@ bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) { bool Match = true; - if (op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { - S.Diag(E->getOperatorLoc(), diag::warn_lunsigned_always_true_comparison) - << "< 0" << "false" << HasEnumType(LHS) - << LHS->getSourceRange() << RHS->getSourceRange(); - } else if (op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { - S.Diag(E->getOperatorLoc(), diag::warn_lunsigned_always_true_comparison) - << ">= 0" << "true" << HasEnumType(LHS) - << LHS->getSourceRange() << RHS->getSourceRange(); - } else if (op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { - S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison) - << "0 >" << "false" << HasEnumType(RHS) - << LHS->getSourceRange() << RHS->getSourceRange(); - } else if (op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { - S.Diag(E->getOperatorLoc(), diag::warn_runsigned_always_true_comparison) - << "0 <=" << "true" << HasEnumType(RHS) - << LHS->getSourceRange() << RHS->getSourceRange(); + if (Op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { + S.Diag(E->getOperatorLoc(), + HasEnumType(LHS) ? diag::warn_lunsigned_enum_always_true_comparison + : diag::warn_lunsigned_always_true_comparison) + << "< 0" << false << LHS->getSourceRange() << RHS->getSourceRange(); + } else if (Op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { + S.Diag(E->getOperatorLoc(), + HasEnumType(LHS) ? diag::warn_lunsigned_enum_always_true_comparison + : diag::warn_lunsigned_always_true_comparison) + << ">= 0" << true << LHS->getSourceRange() << RHS->getSourceRange(); + } else if (Op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { + S.Diag(E->getOperatorLoc(), + HasEnumType(RHS) ? diag::warn_runsigned_enum_always_true_comparison + : diag::warn_runsigned_always_true_comparison) + << "0 >" << false << LHS->getSourceRange() << RHS->getSourceRange(); + } else if (Op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { + S.Diag(E->getOperatorLoc(), + HasEnumType(RHS) ? diag::warn_runsigned_enum_always_true_comparison + : diag::warn_runsigned_always_true_comparison) + << "0 <=" << true << LHS->getSourceRange() << RHS->getSourceRange(); } else Match = false; diff --git a/clang/test/Sema/compare.c b/clang/test/Sema/compare.c index ae26143..97586a7 100644 --- a/clang/test/Sema/compare.c +++ b/clang/test/Sema/compare.c @@ -308,8 +308,59 @@ int rdar8414119_bar(unsigned x) { int rdar8511238() { enum A { A_foo, A_bar }; enum A a; + + if (a == 0) + return 0; + if (a != 0) + return 0; if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 is always false}} - return 0; + return 0; + if (a <= 0) + return 0; + if (a > 0) + return 0; + if (a >= 0) // expected-warning {{comparison of unsigned enum expression >= 0 is always true}} + return 0; + + if (0 == a) + return 0; + if (0 != a) + return 0; + if (0 < a) + return 0; + if (0 <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}} + return 0; + if (0 > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}} + return 0; + if (0 >= a) + return 0; + + if (a == 0U) + return 0; + if (a != 0U) + return 0; + if (a < 0U) // expected-warning {{comparison of unsigned enum expression < 0 is always false}} + return 0; + if (a <= 0U) + return 0; + if (a > 0U) + return 0; + if (a >= 0U) // expected-warning {{comparison of unsigned enum expression >= 0 is always true}} + return 0; + + if (0U == a) + return 0; + if (0U != a) + return 0; + if (0U < a) + return 0; + if (0U <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}} + return 0; + if (0U > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}} + return 0; + if (0U >= a) + return 0; + return 20; } diff --git a/clang/test/Sema/tautological-unsigned-enum-zero-compare.c b/clang/test/Sema/tautological-unsigned-enum-zero-compare.c new file mode 100644 index 0000000..a0c2a30 --- /dev/null +++ b/clang/test/Sema/tautological-unsigned-enum-zero-compare.c @@ -0,0 +1,46 @@ +// RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-enum-zero-compare -verify %s + +int main() { + enum A { A_foo, A_bar }; + enum A a; + +#ifdef TEST + if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 is always false}} + return 0; + if (a >= 0) // expected-warning {{comparison of unsigned enum expression >= 0 is always true}} + return 0; + if (0 <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}} + return 0; + if (0 > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}} + return 0; + if (a < 0U) // expected-warning {{comparison of unsigned enum expression < 0 is always false}} + return 0; + if (a >= 0U) // expected-warning {{comparison of unsigned enum expression >= 0 is always true}} + return 0; + if (0U <= a) // expected-warning {{comparison of 0 <= unsigned enum expression is always true}} + return 0; + if (0U > a) // expected-warning {{comparison of 0 > unsigned enum expression is always false}} + return 0; +#else + // expected-no-diagnostics + if (a < 0) + return 0; + if (a >= 0) + return 0; + if (0 <= a) + return 0; + if (0 > a) + return 0; + if (a < 0U) + return 0; + if (a >= 0U) + return 0; + if (0U <= a) + return 0; + if (0U > a) + return 0; +#endif + + return 1; +} -- 2.7.4