Re: build deps

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Oct 16, 2012, 07:37 UTC
Message-ID: <507D0EC5.4030206@drmicha.warpmail.net>
In-Reply-To: <CAJDDKr5QaByGg0wW_fwFErRyMA6hbg2kHskxOqY4Z_rLoo1maQ@mail.gmail.com>

David Aguilar venit, vidit, dixit 16.10.2012 03:39:

Show 39 quoted lines

> On Mon, Oct 15, 2012 at 1:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@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
>>> mind?
>>
>> 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:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/184908/focus=185014
> 
> 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?:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/206413/focus=206539
> 
> 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.
> 
> cheers,

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.

Michael