David Aguilar venit, vidit, dixit 16.10.2012 03:39: > On Mon, Oct 15, 2012 at 1:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >> Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html