<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 12 October 2017 at 15:41, Roman Lebedev via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Fri, Oct 13, 2017 at 1:22 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On 12 October 2017 at 15:11, Roman Lebedev via Phabricator via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> lebedev.ri reopened this revision.<br>
>> lebedev.ri added a comment.<br>
>> This revision is now accepted and ready to land.<br>
>><br>
>> Reverted due to <a href="http://bb9.pgr.jp/#/builders/20/builds/59" rel="noreferrer" target="_blank">http://bb9.pgr.jp/#/builders/<wbr>20/builds/59</a> that i don't<br>
>> currently know how to deal with.<br>
>> It is really sad that i failed to encounter it during testing.<br>
><br>
><br>
> I see three issues there:<br>
<br>
> 1) A warning in this code due to missing parentheses around a ^ operator.<br>
<br>
> 2) This code generating correct warnings in the libc++ test suite.<br>
</span>Yes, this one is the problem.<br>
<br>
I'm honestly not sure about these comparisons with<br>
std::numeric_limits<...>::{<wbr>min,max}()<br>
They is similar to what Nico Weber (CC'd, just in case) is raising in<br>
post-review mail in<br>
<a href="https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171009/206427.html" rel="noreferrer" target="_blank">https://lists.llvm.org/<wbr>pipermail/cfe-commits/Week-of-<wbr>Mon-20171009/206427.html</a><br>
I personally would very much prefer to have the warning, as explained<br>
in the follow-up mail.</blockquote><div><br></div><div>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.</div><div><br></div><div>But even if the warning ends up off by default, we should still have it available.</div><div><div><br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">> You could ask EricWF (cc'd) to look at those and either fix them or turn the warning<br>
> flag off for libc++'s tests.<br>
</span>Eric: could you *please* look into that? :)<br>
That is way too deep to change without prior knowledge about the code i think.<br>
<span class="gmail-im gmail-HOEnZb"><br>
> 3) A stage2 / stage3 comparison failure in CGAtomic.cpp. That's pre-existing<br>
> and nothing to do with your change.<br>
<br>
</span><span class="gmail-HOEnZb"><font color="#888888">Roman.<br>
</font></span><div class="gmail-HOEnZb"><div class="gmail-h5">______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>