smeenai 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. 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. I guess you can think of it both ways, but the concern for https://reviews.llvm.org/D39149 was definitely that of a false positive. More precisely, a `long` variable was being compared against the limits for `int`. On a 64-bit platform, you would have no warning. On a 32-bit platform, you would have a warning, by virtue of `int` and `long` being the same size. I consider the warning on the 32-bit platform to be a false positive, since the comparison is serving a purpose, but the compiler is still flagging it as tautological. In general, I would want is for `-Wtautological-constant-compare` to only fire in cases where the comparison is **guaranteed** to be tautological, and would consider anything else to be a false positive. 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
