[PATCH] D39149: [libc++] Prevent tautological comparisons

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D39149#910891, @bcain wrote:

> In https://reviews.llvm.org/D39149#910845, @lebedev.ri wrote:
>
> > That is my diagnostic, so i guess this is the time to reply :)
>
>
> ...
>
> > 3. In `-Wtautological-constant-compare`, ignore any comparisons that compare with `std::numeric_limits`. Not a fan of this solution. <- Worst?
>
> ...
>
> Gee, #3 is the worst?  That one seems the most appealing to me.  I think I could understand it if you had reservations about ignoring comparisons against {CHAR,SHRT,INT,LONG}_{MIN,MAX} but std::numeric_limits is well-qualified and everything.
>
> Can you help me understand why you don't prefer it?


That my initial bug was not caught by gcc either, even though `-Wtype-limits` was enabled.
Because for whatever reason gcc basically does not issue diagnostics for templated functions.
Even though the tautologically-compared variable was *not* dependent on the template parameters in any way.
See for yourself, i think this is *really* bad: https://godbolt.org/g/zkq3UD

So all-size-fits-all things like that are just asking to be done wrong.
I fail to see how `ignore any comparisons that compare with std::numeric_limits` would not be the same pitfall, unfortunately.

> Also, is there any way that the warning could somehow be smart enough to know what sizeof(int) and sizeof(long) are?

I'm not sure i follow. It sure does know that, else it would not possible to diagnose anything.
Or were you talking about

In https://reviews.llvm.org/D39149#910845, @lebedev.ri wrote:

> That is my diagnostic, so i guess this is the time to reply :)
>  As i see it, there are several options:
>  ...
>
> 5. The essential problem, i *think* is much like the problem with unreachable code diagnostic, CppCon 2017: Titus Winters “Hands-On With Abseil” <https://youtu.be/xu7q8dGvuwk?t=16m5s>. The check does not know/care whether the comparison is tautological only with the current Fundamental type sizes, or always. Perhaps this is the proper heading?


?

> As an aside, how widely has this new clang behavior been tested?  Is it possible that this new warning behavior might get rolled back if a big/important enough codebase triggers it?

Are you talking about false-positives* or about real true-positive warnings?
So far, to the best of my knowledge, there were no reports of false-positives*.
If a false-positive is reported, and it is not obvious how to fix it, i suppose it might be reverted.
True-positives are obviously not the justification for the revert, but a validation of the validity of the diagnostic.
The cases like this one are troublesome. **If** it is possible to reduce a case that obviously should not warn, **then** the diagnostic should be adjusted not to warn.

- The fact that under *different* circumstances the comparison is not tautological is not a false-positive.


https://reviews.llvm.org/D39149