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