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

[David Blaikie]

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.


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

- 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
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150324/8bd5c9bc/attachment.html>