[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:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Mar 24, 2015 at 9:34 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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.
>
>
> 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.
>
> Happy to have an llvm-dev discussion about what our policy should be here in
> generaly.

Likewise!

>
>>
>> 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.
>
>
> 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.

Agreed. :-)

~Aaron

>
> - David
>
>>
>> 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
>> >> >
>> >> >
>> >
>> >
>
>