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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri 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) ||
----------------
rsmith wrote:
> 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;
> ```
Oh, that looks better indeed :)
I did consider doing something like that, but my variant ended up looking worse than even the current `if()`'s


================
Comment at: lib/Sema/SemaChecking.cpp:8940
+  } else if (!T->hasUnsignedIntegerRepresentation())
+    IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
+
----------------
rsmith wrote:
> 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.
If we look at this code closely, if `hasUnsignedIntegerRepresentation() == false`, we always do `return AnalyzeImpConvsInComparison(S, E);`, regardless of `E->isIntegerConstantExpr(S.Context)`.
So if i move more stuff into `if (T->isIntegralType(S.Context))`, then ` E->isIntegerConstantExpr(S.Context);` is simply gone.
At least the current tests say so.


================
Comment at: lib/Sema/SemaChecking.cpp:8949
+  if (T->isIntegralType(S.Context)) {
+    if (CheckTautologicalComparison(S, E))
+      return AnalyzeImpConvsInComparison(S, E);
----------------
rsmith wrote:
> Pass `LHSValue` and `RHSValue` in here rather than recomputing them.
We can even go one step further, and pass the `bool IsConstLiteralOnRHS`


Repository:
  rL LLVM

https://reviews.llvm.org/D38101