On 12 October 2017 at 15:41, Roman Lebedev via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> On Fri, Oct 13, 2017 at 1:22 AM, Richard Smith <rich...@metafoo.co.uk> > wrote: > > On 12 October 2017 at 15:11, Roman Lebedev via Phabricator via > cfe-commits > > <cfe-commits@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
