[llvm] r233067 - Silencing some MSVC warnings "C4805: '^' : unsafe mix of type 'bool' and type 'unsigned int' in operation"; NFC.

[Aaron Ballman]

On Tue, Mar 24, 2015 at 12:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Mar 24, 2015 at 9:18 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Tue, Mar 24, 2015 at 12:13 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >
>> >
>> > On Tue, Mar 24, 2015 at 5:47 AM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> Author: aaronballman
>> >> Date: Tue Mar 24 07:47:51 2015
>> >> New Revision: 233067
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=233067&view=rev
>> >> Log:
>> >> Silencing some MSVC warnings "C4805: '^' : unsafe mix of type 'bool'
>> >> and
>> >> type 'unsigned int' in operation"; NFC.
>> >
>> >
>> > FWIW, Andy Kaylor's started a review/discussion on disabling some MSVC
>> > warnings and cleaning up others to get a baseline.
>> >
>> > This is one I'm sort of inclined to disable (as mentioned in the review
>> > thread & some other recent discussions - the basic bar that's certainly
>> > been
>> > applied to GCC and I think is a reasonable one is: If the warning
>> > doesn't
>> > meet the quality bar to be added to Clang, then it probably doesn't meet
>> > the
>> > quality bar to be enabled for building LLVM projects and we should just
>> > disable the warning rather than fixing it even once)
>>
>> I've not looked at what code is generated by MSVC for this construct,
>> but when the warning says "unsafe" in it, I get a bit worried. I'd be
>> opposed to silencing this warning if it turns out this really is an
>> unsafe operation with MSVC (though I cannot fathom why it would be
>> since bool can be implicitly converted to int with well-defined
>> semantics). If the operation is safe in MSVC and the warning is more
>> stylistic in nature, then I agree this would be one we can silence
>> globally.
>
>
> Looking at the docs (sparse as they are) on MSDN
> https://msdn.microsoft.com/en-us/library/1hy2y0bk%28v=vs.90%29.aspx perhaps
> this warning was intended for the comparison case & spilled over into
> assignment?
>
> In the comparison case it's still not going to explode on MSVC in
> new/interesting ways, by the sounds of it. It's just seems to be talking
> about "int == bool" comparison promoting the bool to an int, then comparing
> the ints, rather than the other way around, which perhaps the user intended.
>
> None of the first page of Google results for C4805 seem to indicate anything
> more sinister going on here.

Seems like this would be a reasonable one to disable globally then.

FWIW, I somewhat disagree with your previous assertion regarding the
quality bar including Not Even Once for some warnings. Out of tree
projects should compile cleanly, so I think the bar is set differently
for parts of the code base that are likely to be used externally (such
as items in ADT). Turning off warnings entirely the first time we
encounter them makes (small) policy decisions for those use cases if
we're not careful. But we may be able to handle those on an ad hoc
basis easily enough.

~Aaron

>
>>
>>
>> In the meantime, I wanted to get the build back to warning-free.
>>
>> ~Aaron
>>
>> >
>> >>
>> >>
>> >> Modified:
>> >>     llvm/trunk/lib/Support/APFloat.cpp
>> >>
>> >> Modified: llvm/trunk/lib/Support/APFloat.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233067&r1=233066&r2=233067&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Support/APFloat.cpp (original)
>> >> +++ llvm/trunk/lib/Support/APFloat.cpp Tue Mar 24 07:47:51 2015
>> >> @@ -1430,7 +1430,7 @@ APFloat::addOrSubtractSignificand(const
>> >>
>> >>    /* Determine if the operation on the absolute values is effectively
>> >>       an addition or subtraction.  */
>> >> -  subtract ^= sign ^ rhs.sign;
>> >> +  subtract ^= static_cast<bool>(sign ^ rhs.sign);
>> >>
>> >>    /* Are we bigger exponent-wise than the RHS?  */
>> >>    bits = exponent - rhs.exponent;
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>
>