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