[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8667-8679
+  bool Result; // The result of the comparison
+  if ((Op == BO_GT && ValueType == LimitType::Max && ConstOnRight) ||
+      (Op == BO_GT && ValueType == LimitType::Min && !ConstOnRight) ||
+      (Op == BO_LT && ValueType == LimitType::Max && !ConstOnRight) ||
+      (Op == BO_LT && ValueType == LimitType::Min && ConstOnRight)) {
+    Result = false;
+  } else if ((Op == BO_GE && ValueType == LimitType::Max && !ConstOnRight) ||
----------------
The exhaustive case analysis is a little hard to verify. Perhaps something like this would be clearer:

```
bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ ConstOnRight;
bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
bool ResultWhenConstNeOther = ConstIsLowerBound ^ ValueType == LimitType::Max;
if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
  return false;
```


================
Comment at: lib/Sema/SemaChecking.cpp:8940
+  } else if (!T->hasUnsignedIntegerRepresentation())
+    IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
+
----------------
It seems suboptimal to evaluate both sides of the comparison and then evaluate the entire comparison again in this case. Presumably the point is to catch comparisons whose operands are not of integral type (eg, floating point), but we could get that benefit and also catch more integral cases by switching from `isIntegerConstantExpr` to the more appropriate `EvaluateAsRValue`.

I'm fine with a cleanup to avoid the repeated evaluation here being deferred to a later patch.


================
Comment at: lib/Sema/SemaChecking.cpp:8949
+  if (T->isIntegralType(S.Context)) {
+    if (CheckTautologicalComparison(S, E))
+      return AnalyzeImpConvsInComparison(S, E);
----------------
Pass `LHSValue` and `RHSValue` in here rather than recomputing them.


================
Comment at: test/Sema/tautological-constant-compare.c:1-4
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-constant-compare -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-constant-compare -verify -x c++ %s
----------------
This test makes assumptions about the valid ranges of certain built-in types, so should specify a target triple. (Eg, `-triple x86_64-linux-gnu`)


Repository:
  rL LLVM

https://reviews.llvm.org/D38101