r312750 - [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

[Roman Lebedev via cfe-commits]

On Sat, Sep 9, 2017 at 12:56 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Sep 8, 2017 at 5:49 PM, Hans Wennborg via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>> On Fri, Sep 8, 2017 at 2:26 PM, Friedman, Eli <efriedma at codeaurora.org> wrote:
>>> On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:
>>>>
>>>> On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev <lebedev.ri at gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Interesting. My first thought was to explicitly specify enum as signed:
>>>>>
>>>>> enum MediaDeviceType : signed int {
>>>>> MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
>>>>> MEDIA_DEVICE_TYPE_VIDEO_INPUT,
>>>>> MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
>>>>> NUM_MEDIA_DEVICE_TYPES,
>>>>> };
>>>>>
>>>>> inline bool IsValidMediaDeviceType(MediaDeviceType type) {
>>>>> return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
>>>>> }
>>>>>
>>>>> But it still seem to warn, and *that* sounds like a bug.
>>>>> Please open a new bugreport.
>>>>
>>>> I'm reporting it here :-)
>>>>
>>>>> As for different default signedness, i'm not sure what is there to do.
>>>>> Does
>>>>> not sound like a problem for this diagnostic to intentionally avoid to
>>>>> be honest.
>>>>
>>>> I think it is a problem for this warning. If a user sees the warning
>>>> and removes the "type >= 0" check, thinking that it was unnecessary
>>>> because the compiler told them so, they have now introduced a bug in
>>>> the code.
>>>
>>>
>>> Even if you declare the enum type as signed, it still isn't allowed to
>>> contain a negative number; that's undefined behavior.  You can check for
>>> this using ubsan's -fsanitize=enum.
>>
>> Is it? I thought if you define it as signed, it falls under "For an
>> enumeration whose underlying type is fixed, the values of the
>> enumeration are the values of the underlying type." (c++11 7.2p7)
>>
>> My standards-fu is weak though, so I could be wrong.
>
> The enum values can be signed because of the fixed type, but the
> comparison was enum_val <= 0U, so I believe the usual arithmetic
> conversions will convert the enum value to an *unsigned* value, making
> this a tautological comparison. If the value was 0, then I think the
> comparison would not be tautological because there would be no
> conversion needed.

Once i re-committed D37629, test-clang-msc-x64-on-i686-linux-RA build
broke with that exact problem - the expected diagnostic about enum
was not emitted. So i fixed it up in r313747, so now such diagnostic
about tautological comparison of 0 and enum is properly emitted
regardless of the sign of the enum type.

>> But I think you're right about the case when the underlying type is
>> not specified. Even if it happens to be 'int' on Windows, negative
>> values aren't allowed (unless there are negative enumerators). That
>> doesn't mean it won't happen though, and lots of code isn't UB-clean
>> :-/
>
> Yes, but if the *optimizer* were ever clever enough to realize the
> value cannot be negative, then your code still has a pretty serious
> problem in the face of that same UB.
>
> ~Aaron

Roman.