Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 7, 2011, 21:32 UTC
Message-ID: <20111107213219.GA13537@elie.hsd1.il.comcast.net>
In-Reply-To: <7vlirr1vi5.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

>> [jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,
>>  for symmetry]
>
> Umm, why is this removal of defensive programming practice an improvement?

Checking "p->filed < 0" makes static analyzers complain. There's no clean way I know of[*] to get them to shut up and keep the check. The fundamental question is whether the -Wtype-limits check is worth it or not:

 - on one hand, it catches real bugs, such as the mistaken check for
   negative size_t Ævar mentioned;

 - on the other hand, it trips for cases like this in which the type
   only happened to be unsigned on the build platform.  I consider
   that a gcc bug (and apparently clang shares it) --- see
   <http://bugs.debian.org/615525>.

So, the purpose of this patch was to work around this common bug in static analyzers.

Checking "GREP_HEADER_FIELD_MAX <= p->field" without checking for "p->field < 0", like the original patch did, would be only checking half of what it intends to and not add any real guarantees. And more importantly, it would be distracting to people like me and Andreas who would wonder "why doesn't this check p->field < 0, too?".

I guess the commit message should have said

	grep: remove a defensive check to work around common static analyzer bug

> This part is mostly my code and because I know what I wrote is almost
> always perfect, so I do not think there is any real harm done, but still...

Heh.

[*] There are plenty of cryptic, hackish ways possible, though. :)

	if ((size_t) p->field >= GREP_HEADER_FIELD_MAX)
		die(...);