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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits