[Sema] -Wtautological-constant-compare is too good. Cripple it.


[Sema] -Wtautological-constant-compare is too good. Cripple it.

The diagnostic was mostly introduced in D38101 by me, as a reaction to wasting a lot of time, see 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


lebedevriWed, Jan 3, 12:45 AM
Differential Revision
D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.
rL321690: [GlobalISel][Legalizer] Fix legalization of llvm.smul.with.overflow