Page MenuHomePhabricator

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

Authored by lebedev.ri on Dec 21 2017, 1:14 PM.

Details

Summary

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-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.

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri created this revision.Dec 21 2017, 1:14 PM
dim accepted this revision.Dec 24 2017, 7:49 AM
dim added a subscriber: dim.

Yes, please. Rather sooner than later, the warning itself can be fixed post-6.0.0.

This revision is now accepted and ready to land.Dec 24 2017, 7:49 AM
dim added a comment.Dec 24 2017, 8:45 AM

Actually, having thought about it a little more, if the warning is "rather broken", or even "completely broken", depending on one's point of view, then maybe it is better not have it under -Wextra either? E.g. somebody has to ask for the warning specifically, using -Wtautological-constant-compare, or use -Weverything?

I ask this, because in FreeBSD we have traditionally been using -W, which is (again, historically) an alias for -Wextra. We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

In D41512#963743, @dim wrote:

Actually, having thought about it a little more, if the warning is "rather broken", or even "completely broken", depending on one's point of view, then maybe it is better not have it under -Wextra either?

In it's current form, there has been zero false-positives. All the complaints
are because it is finding an actual issues in the people's code, and people
don't feel like fixing the code because in some other case (data model) the
issue would not be present.

Of course, everyone is welcomed to please do prove me wrong,
and report some actual false-positives that are not data-model-dependant.

E.g. somebody has to ask for the warning specifically, using -Wtautological-constant-compare, or use -Weverything?

I ask this, because in FreeBSD we have traditionally been using -W, which is (again, historically) an alias for -Wextra. We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

What @aaron.ballman said in the mail. No one ever will enable it then.
And since it is not actually broken in the first place, i'm a *bit* hesitant to do that.

Though i totally understand the desire to not have bogus warnings,
and so far the clang/clang-analyzer/clang-tidy is the best at that :)

lebedev.ri edited the summary of this revision. (Show Details)
In D41512#963743, @dim wrote:

We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

OTOH, thank you for bringing this up once again. I did thought previously about
moving this specific warning into it's own flag, but never really looked into it.
But now i did, so hopefully this should be even better now :)

lebedev.ri requested review of this revision.Dec 24 2017, 11:24 AM
rsmith added inline comments.Dec 24 2017, 3:56 PM
include/clang/Basic/DiagnosticGroups.td
440–444

I forget, are these in-range-compare warnings new too, or just the non-unsigned-enum form? It would make sense to me for these two to be subgroups of TautologicalRangeCompare.

441

"tautological-constant-in-range-compare" would make more sense to me, as the complement of "tautological-constant-out-of-range-compare".

Move TautologicalUnsignedZeroCompare and TautologicalUnsignedEnumZeroCompare into TautologicalRangeCompare too, and enable them only in -Wextra.
Please advise re flag name for warn_tautological_constant_compare.

include/clang/Basic/DiagnosticGroups.td
440–444

I do believe all three are new: warn_unsigned_enum_always_true_comparison, warn_unsigned_always_true_comparison, warn_tautological_constant_compare.
I'll move them into TautologicalRangeCompare, and disable them too...

But now we are almost back to square one.
Disabling TautologicalRangeCompare will disable all three type-limit-checking diagnostic, and only the inner two can be enabled separately.
Is that really the intended result? I think warn_tautological_constant_compare should have it's own flag.
But i can not come up with a name for it. Please advise.

441

I did think about it.
To me "tautological-constant-in-range-compare" may sound as if the constant is somewhere in the range,
while i'd say "tautological-constant-range-compare" better highlights the fact that the constant *is* the range limit.
No?

lebedev.ri edited the summary of this revision. (Show Details)
  • 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.
  • Slightly adjust (enhance) test coverage

Ping. Branching is approaching rapidly. This needs to land before that.

aaron.ballman added inline comments.Jan 2 2018, 9:52 AM
include/clang/Basic/DiagnosticGroups.td
441

I don't have strong opinions on this name, but have a slight preference for "in range" because I'm left wondering how "range compare" relates to "out of range compare".

smeenai added inline comments.Jan 2 2018, 9:54 AM
include/clang/Basic/DiagnosticGroups.td
441

How about range-limit-compare or range-edge-compare or range-bound-compare or something of that nature, that explicitly captures the fact that it's the range limit?

@aaron.ballman, @smeenai, thank you for feedback!

include/clang/Basic/DiagnosticGroups.td
438

Any opinion on this one?

441

Ok, def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare", it is then.

aaron.ballman added inline comments.Jan 2 2018, 1:46 PM
include/clang/Basic/DiagnosticGroups.td
441

I think @smeenai may have found a name I like better: tautological-range-limit-compare. @rsmith, what do you think?

lebedev.ri updated this revision to Diff 128462.Jan 2 2018, 1:49 PM
lebedev.ri marked 3 inline comments as done.

Renamed TautologicalRangeCompare to TautologicalInRangeCompare.

Given that previous version of this differential was already previously accepted,
and the fact that this needs to happen before the branch, which is rapidly approaching,
i'm going to proceed to committing this now.

lebedev.ri updated this revision to Diff 128470.Jan 2 2018, 2:14 PM

Actually update the diagnostic flag :)

I'm going to hold off committing just yet, and wait for @rsmith to sign off.

rsmith accepted this revision.Jan 2 2018, 6:09 PM
rsmith added inline comments.
include/clang/Basic/DiagnosticGroups.td
438

I would *almost* prefer "tautological-min-max-compare", but I'd be worried about confusion with the std::min / std::max functions. I think this name is good enough. (Ideally, a name that clearly conveys this is about non-zero limiting values would be better, but that's hard to express.)

This revision is now accepted and ready to land.Jan 2 2018, 6:09 PM

FWIW we build with -Wall -Wextra and we disable this warning since it's in our (chromium's) opinion not useful on large real-world code. So I'm not sure it should be in -Wextra. (Also, I believe clang has historically tried to keep -Wall and -Wextra pretty similar?)

This revision was automatically updated to reflect the committed changes.