[Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

[lebedev.ri]

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?

[lebedev.ri]

Rework as per @jroelofs's suggestion to have just one switch/if cascade that operates on BinaryOperatorKind

[jroelofs]

I'm not sure it's better than writing the if/elseif/elseif/elseif out explicitly :/

[lebedev.ri]

And finish reducing the code by for-range-loop`ing over array + use std::array.

[lebedev.ri]

In D37629#865068, @lebedev.ri wrote:

And finish reducing the code by for-range-loop`ing over array + use std::array.

I will need to fix handling of the second edge-case (comparison with max unsigned value or with min/max for signed values), so having the code this way *might* help..

[aaron.ballman]

lib/Sema/SemaChecking.cpp

8600–8603

I don't think this is an improvement over the old code.

[lebedev.ri]

Aaaand revert back to dump approach with code duplication as requested :)

(For the second part of the fix in another differential, if the reverted approach with struct turns out to be better, i guess i will use it)

[lebedev.ri]

Ping, would be really nice to get this going :)
Would unblock me for working on the second half of the fix for https://bugs.llvm.org/show_bug.cgi?id=34147

[aaron.ballman]

lib/Sema/SemaChecking.cpp

8593–8598

Comments should be complete sentences with capitalization and punctuation (here and elsewhere in the patch).

Also, I can't really understand what this comment is saying.

8596

Should be Cmp per coding standards.

[lebedev.ri]

1. Correctly added TautologicalUnsignedEnumZeroCompare to TautologicalCompare group (sigh)
2. Hopefully fixed spelling of the comments and added a bit better comments
3. Fixed variable names to comply with the coding style.

[aaron.ballman]

lib/Sema/SemaChecking.cpp

8605

I'd drop this comment entirely.

8608

Instead of doing all of this complex logic, why not structure the code like the original and use '?:' to determine which diagnostic id to pass in?

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();

[lebedev.ri]

Simplify CheckTautologicalComparisonWithZero() as per review notes.
Less LOC.

[aaron.ballman]

LGTM!

[lebedev.ri]

Reverted due to buildbot failures.
Need to investigate.

include/clang/Basic/DiagnosticGroups.td

 def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
 def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
 def GNUStatementExpression : DiagGroup<"gnu-statement-expression">;
 def StringCompare : DiagGroup<"string-compare">;
 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,
                                      TautologicalUndefinedCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;

include/clang/Basic/DiagnosticSemaKinds.td

 def note_typecheck_assign_const : Note<
   "%select{"
   "function %1 which returns const-qualified type %2 declared here|"
   "variable %1 declared const here|"
   "%select{non-|}1static data member %2 declared const here|"
   "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<TautologicalUnsignedZeroCompare>;
+def warn_runsigned_always_true_comparison : Warning<
+  "comparison of %0 unsigned expression is always %select{false|true}1">,
+  InGroup<TautologicalUnsignedZeroCompare>;
+def warn_lunsigned_enum_always_true_comparison : Warning<
+  "comparison of unsigned enum expression %0 is always %select{false|true}1">,
+  InGroup<TautologicalUnsignedEnumZeroCompare>;
+def warn_runsigned_enum_always_true_comparison : Warning<
+  "comparison of %0 unsigned enum expression is always %select{false|true}1">,
+  InGroup<TautologicalUnsignedEnumZeroCompare>;
+
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
   InGroup<SignCompare>, DefaultIgnore;
-def warn_lunsigned_always_true_comparison : Warning<
-  "comparison of unsigned%select{| enum}2 expression %0 is always %1">,
-  InGroup<TautologicalUnsignedZeroCompare>;
 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<TautologicalOutOfRangeCompare>;
-def warn_runsigned_always_true_comparison : Warning<
-  "comparison of %0 unsigned%select{| enum}2 expression is always %1">,
-  InGroup<TautologicalUnsignedZeroCompare>;
 def warn_comparison_of_mixed_enum_types : Warning<
   "comparison of two values with different enumeration types"
   "%diff{ ($ and $)|}0,1">,
   InGroup<EnumCompare>;
 def warn_comparison_of_mixed_enum_types_switch : Warning<
   "comparison of two values with different enumeration types in switch statement"
   "%diff{ ($ and $)|}0,1">,
   InGroup<EnumCompareSwitch>;

lib/Sema/SemaChecking.cpp

 
 bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) {
   // Disable warning in template instantiations.
   if (S.inTemplateInstantiation())
     return false;
 
   // bool values are handled by DiagnoseOutOfRangeComparison().
 
-  BinaryOperatorKind op = E->getOpcode();
+  BinaryOperatorKind Op = E->getOpcode();
   if (E->isValueDependent())
     return false;
 
   Expr *LHS = E->getLHS();
   Expr *RHS = E->getRHS();
 
-  bool Match = true;
+  // Result is used for two purposes. If the Result is empty, then we decided
+  // that given comparison is non-tautological, and no warning should emitted.
+  // Alternatively, if the Result is not empty, then it contains the result of
+  // the comparison. Since the comparison is tautological, it always evaluates
+  // to the same result - be it either true or false.
+  llvm::Optional<bool> Result;
+
+  // The comparison has two sides - left and right.
+  // This function is called when some side is a constant and other is a
+  // variable. In Value we store the side which contains the variable.
+  Expr *Value;
+
+  const char *Cmp; // Simplified, pretty-printed comparison string
+  // Similar to E->getOpcodeStr(), but with extra 0 on either LHS or RHS
+
+  if (Op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) {
+    Result = false;
+    Value = LHS;
+    Cmp = "< 0";
+  } else if (Op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) {
+    Result = true;
+    Value = LHS;
+    Cmp = ">= 0";
+  } else if (Op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) {
+    Result = false;
+    Value = RHS;
+    Cmp = "0 >";
+  } else if (Op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) {
+    Result = true;
+    Value = RHS;
+    Cmp = "0 <=";
+  }
 
-  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();
-  } else
-    Match = false;
+  if (!Result)
+    return false;
+
+  const bool ValueHasEnumType = HasEnumType(Value);
+  enum IsEnum { False, True, SizeOfIsEnum };
+  static const struct Diagnostics {
+    unsigned BO_LT_OP[SizeOfIsEnum];
+    unsigned BO_GE_OP[SizeOfIsEnum];
+    unsigned BO_GT_OP[SizeOfIsEnum];
+    unsigned BO_LE_OP[SizeOfIsEnum];
+  } DiagnosticsTable = {
+      {diag::warn_lunsigned_always_true_comparison,
+       diag::warn_lunsigned_enum_always_true_comparison},
+      {diag::warn_lunsigned_always_true_comparison,
+       diag::warn_lunsigned_enum_always_true_comparison},
+      {diag::warn_runsigned_always_true_comparison,
+       diag::warn_runsigned_enum_always_true_comparison},
+      {diag::warn_runsigned_always_true_comparison,
+       diag::warn_runsigned_enum_always_true_comparison},
+  };
+
+  unsigned DiagID;
+  switch (Op) {
+  case BO_LT:
+    DiagID = DiagnosticsTable.BO_LT_OP[ValueHasEnumType];
+    break;
+  case BO_GE:
+    DiagID = DiagnosticsTable.BO_GE_OP[ValueHasEnumType];
+    break;
+  case BO_GT:
+    DiagID = DiagnosticsTable.BO_GT_OP[ValueHasEnumType];
+    break;
+  case BO_LE:
+    DiagID = DiagnosticsTable.BO_LE_OP[ValueHasEnumType];
+    break;
+  default:
+    llvm_unreachable("can't get here");
+  }
+
+  S.Diag(E->getOperatorLoc(), DiagID)
+      << Cmp << *Result << LHS->getSourceRange() << RHS->getSourceRange();
 
-  return Match;
+  return true;
 }
 
 void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant,
                                   Expr *Other, const llvm::APSInt &Value,
                                   bool RhsConstant) {
   // Disable warning in template instantiations.
   if (S.inTemplateInstantiation())
     return;

test/Sema/compare.c

int rdar8414119_bar(unsigned x) {
   return x >= ZERO; // no-warning
 }
 #undef ZERO
 #undef CHECK
 
 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;
+  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;
 }
 
 // PR10336
 int test9(int sv, unsigned uv, long slv) {
   return sv == (uv ^= slv); // expected-warning {{comparison of integers of different signs: 'int' and 'unsigned int'}}
 }
 

test/Sema/tautological-unsigned-enum-zero-compare.c

+// 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;
+}