[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:8583-8586
+// Checks whether Expr 'Constant' may be the
+// std::numeric_limits<>::max() or std::numeric_limits<>::min()
+// of the Expr 'Other'. If true, then returns the limit type (min or max).
+// Does not consider 0 to be type limit. IsZero() and friends do that already.
----------------
Use `///` for documentation comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8586
+// of the Expr 'Other'. If true, then returns the limit type (min or max).
+// Does not consider 0 to be type limit. IsZero() and friends do that already.
+llvm::Optional<LimitType> IsTypeLimit(Sema &S, Expr *Other, Expr *Constant,
----------------
Is there a reason to separate the checks for 0 from the other checks for min/max values? It looks like the code below would be slightly simpler if the two checks were combined.


================
Comment at: lib/Sema/SemaChecking.cpp:8588
+llvm::Optional<LimitType> IsTypeLimit(Sema &S, Expr *Other, Expr *Constant,
+                                      llvm::APSInt *Value) {
+  if (IsEnumConstOrFromMacro(S, Constant))
----------------
`Value` can't be null; pass it by reference instead.


================
Comment at: lib/Sema/SemaChecking.cpp:8636-8637
+bool isNonBooleanIntegerValue(Expr *E) {
+  // We are checking that the expression is not known to have boolean value,
+  // is an integer type
+  return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType();
----------------
This comment adds nothing to the following code; remove?


================
Comment at: lib/Sema/SemaChecking.cpp:8720-8768
+  if (Op == BO_LT && isNonBooleanIntegerValue(RHS) &&
+      IsTypeLimit(S, RHS, LHS, &Value) == LimitType::Max) {
+    OS << Value;
+    S.Diag(E->getOperatorLoc(), diag::warn_tautological_lconstant_compare)
+        << RType << OpStr << OS.str() << false << LHS->getSourceRange()
+        << RHS->getSourceRange();
+  } else if (Op == BO_LT && isNonBooleanIntegerValue(LHS) &&
----------------
Please try to reduce/factor out the duplication here.


================
Comment at: lib/Sema/SemaChecking.cpp:8796
+  llvm::APSInt ConstValue;
+  // type limit values are handled later by CheckTautologicalComparisonWithMinMax().
+  if (IsTypeLimit(S, Other, Constant, &ConstValue) && (!OtherIsBooleanType))
----------------
Comments should start with a capital letter.


Repository:
  rL LLVM

https://reviews.llvm.org/D38101