[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Richard Smith via cfe-commits]

On 12 October 2017 at 15:41, Roman Lebedev via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Fri, Oct 13, 2017 at 1:22 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On 12 October 2017 at 15:11, Roman Lebedev via Phabricator via
> cfe-commits
> > <cfe-commits at lists.llvm.org> wrote:
> >>
> >> lebedev.ri reopened this revision.
> >> lebedev.ri added a comment.
> >> This revision is now accepted and ready to land.
> >>
> >> Reverted due to http://bb9.pgr.jp/#/builders/20/builds/59 that i don't
> >> currently know how to deal with.
> >> It is really sad that i failed to encounter it during testing.
> >
> >
> > I see three issues there:
>
> > 1) A warning in this code due to missing parentheses around a ^ operator.
>
> > 2) This code generating correct warnings in the libc++ test suite.
> Yes, this one is the problem.
>
> I'm honestly not sure about these comparisons with
> std::numeric_limits<...>::{min,max}()
> They is similar to what Nico Weber (CC'd, just in case) is raising in
> post-review mail in
> https://lists.llvm.org/pipermail/cfe-commits/Week-of-
> Mon-20171009/206427.html
> I personally would very much prefer to have the warning, as explained
> in the follow-up mail.


Our general philosophy on such things is: if there's some pattern of false
positives (ie, cases where the code is reasonable and intentionally
performing a tautological comparison) that we can reasonably identify, then
disabling the warning for those cases would be a good idea. If the rate of
false positives is not very low and we can't identify the problematic
patterns, then we should turn the warning off by default.

But even if the warning ends up off by default, we should still have it
available.

> You could ask EricWF (cc'd) to look at those and either fix them or turn
> the warning
> > flag off for libc++'s tests.
> Eric: could you *please* look into that? :)
> That is way too deep to change without prior knowledge about the code i
> think.
>
> > 3) A stage2 / stage3 comparison failure in CGAtomic.cpp. That's
> pre-existing
> > and nothing to do with your change.
>
> Roman.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171012/92793659/attachment-0001.html>