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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks, this looks great.



================
Comment at: lib/Sema/SemaChecking.cpp:8940
+  } else if (!T->hasUnsignedIntegerRepresentation())
+    IsComparisonConstant = E->isIntegerConstantExpr(S.Context);
+
----------------
lebedev.ri wrote:
> 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.
It looks like the old behavior was to suppress the `CheckTautologicalComparisonWithZero` diagnostics when:

* the comparison has a constant value, and
* the types being compared are not integral types, and
* the types being compared do not have an unsigned integer representation

However, `CheckTautologicalComparisonWithZero` only emits diagnostics for comparisons with integral types. So I think you're right that the old codepath that evaluated `E->isIntegerConstantExpr(S.Context)` was pointless.


================
Comment at: lib/Sema/SemaChecking.cpp:8924
 
-  // If this is a tautological comparison, suppress -Wsign-compare.
-  if (CheckTautologicalComparisonWithZero(S, E))
-    return AnalyzeImpConvsInComparison(S, E);
+    bool IsComparisonConstant = (IsRHSIntegralLiteral && IsLHSIntegralLiteral);
+    // We don't care about expressions whose result is a constant.
----------------
I don't think this variable pulls its weight any more, especially given the adjacent comment. Inline its initializer into the `if` condition, maybe?


================
Comment at: lib/Sema/SemaChecking.cpp:8930
+    // We only care about expressions where just one side is literal
+    if (IsRHSIntegralLiteral ^ IsLHSIntegralLiteral) {
+      // Is the constant on the RHS or LHS?
----------------
It would probably be more obvious to use `||` here, since you already returned in the case where both sides are constant.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101