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

[Hans Wennborg via cfe-commits]

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