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

[Brian Cain via Phabricator via cfe-commits]

bcain added a comment.

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

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


...

Ok, yes, that makes sense.

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

Yes, sorry, I had the logic reversed in my head.

...

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

Isn't this a case that it obviously should not warn?

What about another approach: (1) could we consider Shoaib's suggestion to keep the exhaustive check with this name and create a new one that's limited (and can take the current one's place in the enabled-by-Wall) or (2) could we create range check functions that have the warning disabled?

Option #2 is limited to benefiting only llvm or libcxx code.  But if we narrow our focus to just the llvm/libcxx code for the time being, we should have a common idiom for resolving this problem.  It's likely to show up elsewhere too.  The fact that we were considering ifdefs here versus the pragmas in the earlier commit is concerning.


https://reviews.llvm.org/D39149