rjmccall added a comment. 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. We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't. 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. > 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. Repository: rL LLVM https://reviews.llvm.org/D39462 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
