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