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

[Aaron Ballman via cfe-commits]

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.

> 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