r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

On Fri, Dec 8, 2017 at 11:10 AM, Hans Wennborg <hans at chromium.org> wrote:
> On Fri, Dec 8, 2017 at 9:30 AM, Richard Smith via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>> On 8 Dec 2017 08:54, "Hans Wennborg via cfe-commits"
>> <cfe-commits at lists.llvm.org> wrote:
>>
>> Author: hans
>> Date: Fri Dec  8 08:54:08 2017
>> New Revision: 320162
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=320162&view=rev
>> Log:
>> Revert "Unify implementation of our two different flavours of
>> -Wtautological-compare."
>>
>>> Unify implementation of our two different flavours of
>>> -Wtautological-compare.
>>>
>>> In so doing, fix a handful of remaining bugs where we would report false
>>> positives or false negatives if we promote a signed value to an unsigned
>>> type
>>> for the comparison.
>>
>> This caused a new warning in Chromium:
>>
>> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant
>> 64
>> with expression of type 'unsigned int' is always true
>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>   DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
>>          ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> The 'unsigned int' is really a 6-bit bitfield, which is why it's always
>> less than 64.
>>
>> I thought we didn't use to warn (with out-of-range-compare) when comparing
>> against the boundaries of a type?
>>
>>
>> This isn't a "boundary of the type" case, though -- 64 is out of range for a
>> 6 bit bitfield. Your CHECK does nothing. Do you think that it's not
>> reasonable to warn on this bug, or that it's not a bug?
>
> I'm not sure it's a bug; and the CHECK will fire in case someone
> widens that bitfield and stores a larger value into it.
>
> I suppose I should rewrite the code as "handle.event_index <=
> TraceBufferChunk::kTraceBufferChunkSize -1"?
>
> The reason I thought this was unintentional is because I thought we
> don't warn in cases like:
>
> void f(int x) {
>         if (x <= INT_MAX) {
>                 foo();
>         }
> }
>
> and that's essentially what the code is doing, except it's using <
> instead of <=.

We do warn for

void f(int x) {
        if (x < (long long)INT_MAX + 1) {
                foo();
        }
}

so I suppose your patch is consistent with the existing warning.

Sorry for reverting, go ahead and reland or let me know if you'd like
me to do it.