Page MenuHomePhabricator

Add -Wtautological-value-range-compare warning.
ClosedPublic

Authored by rsmith on Aug 4 2020, 4:21 PM.

Details

Summary

This warning diagnoses cases where an expression is compared to a
constant, and the comparison is tautological due to the form of the
expression (but not merely due to its type). This applies in cases such
as comparisons of bit-fields and the result of bit-masks.

The new warning is added to the Clang diagnostic group
-Wtautological-constant-in-range-compare but not to the
formerly-equivalent GCC-compatibility diagnostic group -Wtype-limits,
which retains its old meaning of diagnosing only tautological
comparisons to extremal values of a type (eg, int > INT_MAX).

Diff Detail

Event Timeline

rsmith created this revision.Aug 4 2020, 4:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2020, 4:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith requested review of this revision.Aug 4 2020, 4:21 PM
rsmith updated this revision to Diff 283070.Aug 4 2020, 5:21 PM

Unbreak test that was autobroken by lint.

rsmith updated this revision to Diff 283072.Aug 4 2020, 5:25 PM
This comment was removed by rsmith.
rsmith added a comment.Aug 4 2020, 5:33 PM

Not directly; GetExprRange in SemaChecking apparently intentionally treats casts as producing an arbitrary value of the destination type:

// I think we only want to look through implicit casts here; if the
// user has an explicit widening cast, we should treat the value as
// being of the new, wider type.

I'm not entirely convinced that what the comment describes is the best approach. If we revisited that choice then, with this patch, we would indeed diagnose the PR40921 case. But I think that and this are separate changes and I'd prefer not to combine them.

But I think that and this are separate changes and I'd prefer not to combine them.

Yeah, right.

rtrieu accepted this revision.Aug 6 2020, 4:17 AM

LGTM

This revision is now accepted and ready to land.Aug 6 2020, 4:17 AM
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Aug 11 2020, 1:46 AM

I think this generates a false positive with test.cc

enum E { E1 = 1, E2 = 2 };
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }

and clang++ -fsyntax-only -Wtautological-value-range-compare test.cc

test.cc:2:62: warning: result of comparison of 1-bit unsigned value > 1 is always false [-Wtautological-value-range-compare]
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

I think this generates a false positive with test.cc

enum E { E1 = 1, E2 = 2 };
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }

and clang++ -fsyntax-only -Wtautological-value-range-compare test.cc

test.cc:2:62: warning: result of comparison of 1-bit unsigned value > 1 is always false [-Wtautological-value-range-compare]
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

This appears to be a general problem: the GetExprRange mechanism in SemaChecking miscomputes the ranges for +, *, and - expressions, and we'll get them wrong for all warnings that use that mechanism :-(

This appears to be a general problem: the GetExprRange mechanism in SemaChecking miscomputes the ranges for +, *, and - expressions, and we'll get them wrong for all warnings that use that mechanism :-(

See https://reviews.llvm.org/D85778 for a fix. We were intentionally miscomputing the expression ranges in order to give conservative narrowing-conversion results for cases like char a = some_char + 1; (which is technically lossy because the RHS could be CHAR_MAX + 1, but is probably fine), and this caused us to similarly compute that (a & 1) + (b & 1) was in the range [0, 1], not [0, 2].