[llvm] r202295 - Silencing an MSVC signed comparison warning.

[Aaron Ballman]

On Wed, Feb 26, 2014 at 4:17 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Feb 26, 2014 at 1:12 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Wed, Feb 26, 2014 at 4:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> >
>> > On Wed, Feb 26, 2014 at 1:01 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> On Wed, Feb 26, 2014 at 3:55 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Wed, Feb 26, 2014 at 12:22 PM, Aaron Ballman
>> >> > <aaron at aaronballman.com>
>> >> > wrote:
>> >> >>
>> >> >> Author: aaronballman
>> >> >> Date: Wed Feb 26 14:22:20 2014
>> >> >> New Revision: 202295
>> >> >>
>> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=202295&view=rev
>> >> >> Log:
>> >> >> Silencing an MSVC signed comparison warning.
>> >> >
>> >> >
>> >> > Do we have a bug tracking Clang's implementation of this? (or turning
>> >> > it
>> >> > on
>> >> > for the LLVM build if it's already implemented)
>> >>
>> >> We have -Wsign-compare which I would imagine does the trick, but I
>> >> believe we would not want it to fire on this code.
>> >>
>> >> > (either that, or can we turn off the MSVC warning if it's not
>> >> > considered
>> >> > high value?)
>> >>
>> >> I think it's usually high value, except in this case where MSVC cannot
>> >> figure out that 32/64 as a literal is "unsigned enough." That's why I
>> >> would claim we wouldn't want Clang to fire a warning here anyway -- we
>> >> can prove the comparison is fine.
>> >
>> >
>> > So - both then: suppress the MSVC warning and enable the Clang one, if
>> > it's
>> > not already enabled?
>>
>> I don't think we should suppress the MSVC warning,
>
>
> This is mostly for your (& other MSVC) developers benefit so your builds
> don't get broken by non-MSVC users... so I don't have any real basis on
> which to object for myself. If that's the sort of thing you're willing to
> accept...
>
> But generally I think the project is of the opinion that "if the warning is
> good enough to use, it's good enough to implement" so we should either
> implement it or disable it.

Understood, but I do believe we already implement it with
-Wsign-compare. So I don't see a reason to disable for MSVC so much as
a reason to enable for LLVM and Clang. If it proves to be really
chatty for us and has more false-positives than MSVC, we should
improve our implementation.

>> but I do think we
>> should enable the Clang one. This comes up infrequently enough in MSVC
>> that I'm not worried about occasionally making the literal type more
>> explicit.
>>
>> I'm not really set up to run a clang bootstrap, so I don't feel
>> comfortable turning the warning on for Clang (and LLVM). Would you
>> mind giving it a whirl to see how chatty it is?
>
>
> I'm going to assume far too chatty - but I'll try to remember to give it a
> go at some point.

Thanks for (someday) giving it a shot!

~Aaron