<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 9:34 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Mar 24, 2015 at 12:23 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Mar 24, 2015 at 9:18 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Mar 24, 2015 at 12:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> wrote:<br>
>> ><br>
>> ><br>
>> > On Tue, Mar 24, 2015 at 5:47 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Author: aaronballman<br>
>> >> Date: Tue Mar 24 07:47:51 2015<br>
>> >> New Revision: 233067<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233067&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=233067&view=rev</a><br>
>> >> Log:<br>
>> >> Silencing some MSVC warnings "C4805: '^' : unsafe mix of type 'bool'<br>
>> >> and<br>
>> >> type 'unsigned int' in operation"; NFC.<br>
>> ><br>
>> ><br>
>> > FWIW, Andy Kaylor's started a review/discussion on disabling some MSVC<br>
>> > warnings and cleaning up others to get a baseline.<br>
>> ><br>
>> > This is one I'm sort of inclined to disable (as mentioned in the review<br>
>> > thread & some other recent discussions - the basic bar that's certainly<br>
>> > been<br>
>> > applied to GCC and I think is a reasonable one is: If the warning<br>
>> > doesn't<br>
>> > meet the quality bar to be added to Clang, then it probably doesn't meet<br>
>> > the<br>
>> > quality bar to be enabled for building LLVM projects and we should just<br>
>> > disable the warning rather than fixing it even once)<br>
>><br>
>> I've not looked at what code is generated by MSVC for this construct,<br>
>> but when the warning says "unsafe" in it, I get a bit worried. I'd be<br>
>> opposed to silencing this warning if it turns out this really is an<br>
>> unsafe operation with MSVC (though I cannot fathom why it would be<br>
>> since bool can be implicitly converted to int with well-defined<br>
>> semantics). If the operation is safe in MSVC and the warning is more<br>
>> stylistic in nature, then I agree this would be one we can silence<br>
>> globally.<br>
><br>
><br>
> Looking at the docs (sparse as they are) on MSDN<br>
> <a href="https://msdn.microsoft.com/en-us/library/1hy2y0bk%28v=vs.90%29.aspx" target="_blank">https://msdn.microsoft.com/en-us/library/1hy2y0bk%28v=vs.90%29.aspx</a> perhaps<br>
> this warning was intended for the comparison case & spilled over into<br>
> assignment?<br>
><br>
> In the comparison case it's still not going to explode on MSVC in<br>
> new/interesting ways, by the sounds of it. It's just seems to be talking<br>
> about "int == bool" comparison promoting the bool to an int, then comparing<br>
> the ints, rather than the other way around, which perhaps the user intended.<br>
><br>
> None of the first page of Google results for C4805 seem to indicate anything<br>
> more sinister going on here.<br>
<br>
</div></div>Seems like this would be a reasonable one to disable globally then.<br>
<br>
FWIW, I somewhat disagree with your previous assertion regarding the<br>
quality bar including Not Even Once for some warnings. </blockquote><div><br>Fair enough - this was the impression that I got early on when we hit a lot of bad GCC warnings that weren't reasonable to uphold, so I've just tried to apply it universally. A regular occurrence were GCC -Wuninitialized warnings which were being suppressed by initializing variables which had a dynamic constraint that guaranteed initialization anyway, this sort of thing could/would hurt tools like MSan and ASan, no longer able to test that the dynamic constraint wasn't violated. So there was a slightly stronger justification in those cases than perhaps there is here.<br><br>Happy to have an llvm-dev discussion about what our policy should be here in generaly.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Out of tree<br>
projects should compile cleanly, so I think the bar is set differently<br>
for parts of the code base that are likely to be used externally (such<br>
as items in ADT). Turning off warnings entirely the first time we<br>
encounter them makes (small) policy decisions for those use cases if<br>
we're not careful.</blockquote><div><br>And in this case specifically - I don't think we really care(d) much about that, but perhaps need a clear decision one way or another & to document what our guarantees are about warning-cleanliness on other compilers.</div><div><br></div><div>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> But we may be able to handle those on an ad hoc<br>
basis easily enough.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> In the meantime, I wanted to get the build back to warning-free.<br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> Modified:<br>
>> >> llvm/trunk/lib/Support/APFloat.cpp<br>
>> >><br>
>> >> Modified: llvm/trunk/lib/Support/APFloat.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233067&r1=233066&r2=233067&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233067&r1=233066&r2=233067&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- llvm/trunk/lib/Support/APFloat.cpp (original)<br>
>> >> +++ llvm/trunk/lib/Support/APFloat.cpp Tue Mar 24 07:47:51 2015<br>
>> >> @@ -1430,7 +1430,7 @@ APFloat::addOrSubtractSignificand(const<br>
>> >><br>
>> >> /* Determine if the operation on the absolute values is effectively<br>
>> >> an addition or subtraction. */<br>
>> >> - subtract ^= sign ^ rhs.sign;<br>
>> >> + subtract ^= static_cast<bool>(sign ^ rhs.sign);<br>
>> >><br>
>> >> /* Are we bigger exponent-wise than the RHS? */<br>
>> >> bits = exponent - rhs.exponent;<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>