On Fri, Dec 8, 2017 at 11:10 AM, Hans Wennborg <h...@chromium.org> wrote:
> On Fri, Dec 8, 2017 at 9:30 AM, Richard Smith via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
>> On 8 Dec 2017 08:54, "Hans Wennborg via cfe-commits"
>> <cfe-commits@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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
