Junio C Hamano wrote: > Jonathan Nieder <jrnieder@xxxxxxxxx> 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(...); -- 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