[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D39462#922847, @rjmccall wrote:

> In https://reviews.llvm.org/D39462#922844, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D39462#922826, @rjmccall wrote:
> >
> > > I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here.  It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives.  I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.
> >
> >
> > Just to reiterate that we are talking about the same thing here:
> >
> > - https://reviews.llvm.org/D38101 is already merged. `-Wtautological-constant-compare` is here.
> > - There are cases when it warns for some target platform, but not the other, as complained in https://reviews.llvm.org/D39149, and post-review mails for https://reviews.llvm.org/D38101
> > - So far it seems all the cases reduce to ``` #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()) {} } ```
> > - *This* differential (https://reviews.llvm.org/D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of `-Wtautological-constant-compare`.
>
>
> I think you might have this backwards.


The views here are clearly polarized.

> We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't.

Yes.

> What you've identified here is a false *negative*, i.e. a case where you believe it should warn (because it would warn on a different target) that it currently does not.

I'm sorry, but where are you getting this from? I don't believe these warnings should be elevated to always warn even if it is not tautological for the current target platform. I don't think i have said that?

>> Are you suggesting me to drop this, and implement some other huge new diagnostic that may catch such cases before `-Wtautological-constant-compare`, thus preventing `-Wtautological-constant-compare` from triggering on that completely?
> 
> "This warning triggers on some targets and not on others" is a far more widespread problem than just -Wtautological-constant-compare.  I don't think your patch reasonably solves that problem, even for just this diagnostic.  I think a "strong typedef" analysis would address it, but that's going to require a significant amount of work, even if you literally only apply it to this warning, because you're going to have to implement a bunch of more careful logic about inferring when to propagate typedefs through e.g. templates, as well as when to consider a template to have "propagated" through arithmetic promotion in the same way we propagate integer ranges.

First and foremost, i do admit that i don't have the full picture in mind.
If other reviewers agree with Your view, i will abandon this differential. Hopefully someone with clearer understanding will come up with a better solution.

In https://reviews.llvm.org/D39462#922887, @rjmccall wrote:

> I see.  The problem now, though, is that things involving, say, a size_t and a numeric_limits<size_t> will never warn.


The same type (`size_t`) will be on the both sides, so i think it should still warn. (I do see that the test disagrees with me.)


Repository:
  rL LLVM

https://reviews.llvm.org/D39462