David Aguilar venit, vidit, dixit 16.10.2012 03:39:
> On Mon, Oct 15, 2012 at 1:53 PM, Junio C Hamano <gits...@pobox.com> wrote:
>> Michael J Gruber <g...@drmicha.warpmail.net> writes:
>>>> grep.c:451:16: warning: comparison of unsigned enum expression < 0 is
>>>> always false [-Wtautological-compare]
>>>> if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
>>>> ~~~~~~~~ ^ ~
>>>> 1 warning generated.
>>> Right, that enum type starts at 0. Junio, you last touched this area.
>>> Can we just dump the first comparison or did you have something else in
>> I think it was a leftover from the very first implementation that
>> defensively said "this has to be one of these known ones", and tried
>> to bound it from both sides of the range, regaredless of the actual
>> type of the field (these GREP_HEADER_WHAT things may have been
>> simple integers with #define'd values). Dropping the "negative"
>> comparison is perfectly fine.
> This snippet of code came up before:
> There seemed to be good reasons to keep the check at the time.
> Was this same snippet not also touched when Nguyen Thai Ngoc Duy
> worked on the "even if I'm drunk" patch?:
> With the "drunk" patch then we wouldn't need the check at all,
> which is really nice.
> I hope that helps jog folks' memories.
> I'm not sure if the above discussions are relevant anymore,
> but I figured it'd be good to provide some more context.
The drunk patch, cheers ;)
That's very valuable context that you are giving. So it's either
avoiding the warning and relying and enum unsignedness (or human/static
analysis) or playing it safe and keeping the warning. How is
if (/* p->field < 0 || */ GREP_HEADER_FIELD_MAX <= p->field)
to remind any reader that the first condition should be granted? One
could take this further and use a macro but that seems overkill.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html