Summary:
The diagnostic was mostly introduced in D38101 by me, as a reaction to wasting a lot of time, see [[ https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171009/206427.html | mail ]].
However, the diagnostic is pretty dumb. While it works with no false-positives,
there are some questionable cases that are diagnosed when one would argue that they should not be.
The common complaint is that it diagnoses the comparisons between an `int` and
`long` when compiling for a 32-bit target as tautological, but not when
compiling for 64-bit targets. The underlying problem is obvious: data model.
In most cases, 64-bit target is `LP64` (`int` is 32-bit, `long` and pointer are
64-bit), and the 32-bit target is `ILP32` (`int`, `long`, and pointer are 32-bit).
I.e. the common pattern is: (pseudocode)
```
#include <limits>
#include <cstdint>
int main() {
using T1 = long;
using T2 = int;
T1 r;
if (r < std::numeric_limits<T2>::min()) {}
if (r > std::numeric_limits<T2>::max()) {}
}
```
As an example, D39149 was trying to fix this diagnostic in libc++, and it was not well-received.
This *could* be "fixed", by changing the diagnostics logic to something like
`if the types of the values being compared are different, but are of the same size, then do diagnose`,
and i even attempted to do so in D39462, but as @rjmccall rightfully commented,
that implementation is incomplete to say the least.
So to stop causing trouble, and avoid contaminating upcoming release, lets do this workaround:
* move these three diags (`warn_unsigned_always_true_comparison`, `warn_unsigned_enum_always_true_comparison`, `warn_tautological_constant_compare`) into it's own `-Wtautological-constant-in-range-compare`
* Disable them by default
* Make them part of `-Wextra`
* Additionally, give `warn_tautological_constant_compare` it's own flag `-Wtautological-type-limit-compare`.
I'm not happy about that name, but i can't come up with anything better.
This way all three of them can be enabled/disabled either altogether, or one-by-one.
Reviewers: aaron.ballman, rsmith, smeenai, rjmccall, rnk, mclow.lists, dim
Reviewed By: aaron.ballman, rsmith, dim
Subscribers: thakis, compnerd, mehdi_amini, dim, hans, cfe-commits, rjmccall
Tags: #clang
Differential Revision: https://reviews.llvm.org/D41512
llvm-svn: 321691
This mimics FileCheck's --check-prefixes option.
The default prefix is "expected". That is, "-verify" is equivalent to
"-verify=expected".
The goal is to permit exercising a single test suite source file with different
compiler options producing different sets of diagnostics. While cpp can be
combined with the existing -verify to accomplish the same goal, source is often
easier to maintain when it's not cluttered with preprocessor directives or
duplicate passages of code. For example, this patch also rewrites some existing
clang tests to demonstrate the benefit of this feature.
Patch by Joel E. Denny, thanks!
Differential Revision: https://reviews.llvm.org/D39694
llvm-svn: 320908
Summary:
As Mattias Eriksson has reported in PR35009, in C, for enums, the underlying type should
be used when checking for the tautological comparison, unlike C++, where the enumerator
values define the value range. So if not in CPlusPlus mode, use the enum underlying type.
Also, i have discovered a problem (a crash) when evaluating tautological-ness of the following comparison:
```
enum A { A_a = 0 };
if (a < 0) // expected-warning {{comparison of unsigned enum expression < 0 is always false}}
return 0;
```
This affects both the C and C++, but after the first fix, only C++ code was affected.
That was also fixed, while preserving (i think?) the proper diagnostic output.
And while there, attempt to enhance the test coverage.
Yes, some tests got moved around, sorry about that :)
Fixes PR35009
Reviewers: aaron.ballman, rsmith, rjmccall
Reviewed By: aaron.ballman
Subscribers: Rakete1111, efriedma, materi, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D39122
llvm-svn: 316268
As Aaron Ballman has pointed out, that is not really correct.
So the key problem there is the invalidity of the testcase.
Revert r313747, and rework testcase in such a way, so these
details (platform-specific default enum sigdness) are
accounted for.
Also, add a C++-specific testcase.
llvm-svn: 313756
Recommit. Original commit was reverted because buildbots broke.
The error was only reproducible in the build with assertions.
The problem was that the diagnostic expected true/false as
bool, while it was provided as string "true"/"false".
Summary:
As requested by Sam McCall:
> Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
> The warning strongly suggests that the enum < 0 check has no effect
> (for enums with nonnegative ranges).
> Clang doesn't seem to optimize such checks out though, and they seem
> likely to catch bugs in some cases. Yes, only if there's UB elsewhere,
> but I assume not optimizing out these checks indicates a deliberate
> decision to stay somewhat compatible with a technically-incorrect
> mental model.
> If this is the case, should we move these to a
> -Wtautological-compare-enum subcategory?
Reviewers: rjmccall, rsmith, aaron.ballman, sammccall, bkramer, djasper
Reviewed By: aaron.ballman
Subscribers: jroelofs, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D37629
llvm-svn: 313745
Summary:
As requested by Sam McCall:
> Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
> The warning strongly suggests that the enum < 0 check has no effect
> (for enums with nonnegative ranges).
> Clang doesn't seem to optimize such checks out though, and they seem
> likely to catch bugs in some cases. Yes, only if there's UB elsewhere,
> but I assume not optimizing out these checks indicates a deliberate
> decision to stay somewhat compatible with a technically-incorrect
> mental model.
> If this is the case, should we move these to a
> -Wtautological-compare-enum subcategory?
Reviewers: rjmccall, rsmith, aaron.ballman, sammccall, bkramer, djasper
Reviewed By: aaron.ballman
Subscribers: jroelofs, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D37629
llvm-svn: 313677
Roman Lebedev