Re: [PATCH v2] cache_tree_find(): remove redundant checks

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2014, 23:18 UTC
Message-ID: <xmqqr46himd1.fsf@gitster.dls.corp.google.com>
In-Reply-To: <531652B9.4000001@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> Isn't the above a strchrnul()?
>
> Oh, cool, I never realized that this GNU extension was blessed for use
> in Git.  Will change.

We do have our own fallbacks for non-glibc platforms, so it should be safe.

Show 11 quoted lines

>> Combining a freestanding decl with intializer assignment to lose one
>> line is sort of cheating on the line count, but replacing the three
>> lines with a single strchrnul() would be a real code reduction ;-)
>
> I suppose you are just teasing me, but for the record I consider line
> count only a secondary metric.  The reason for combining initialization
> with declaration is to reduce by one the number of times that the reader
> has to think about that variable when analyzing the code.
> ...
> I really wish we could mix declarations with statements because I think
> it is a big help to readability.

Unfortunately, I think we are in violent disagreement.

A variable declaration block with initializations on only some but not all variables is extremely annoying. If none of the variable declaration has initialization (or initialization to trivial values that do not depend on the logic flow), and the first statement is separated from the decl block, then I do not have to read the decl part when reading the code/logic *at all* (the compiler will find missing variables, variables declared as a wrong type, etc.).

In other words, a trivial initialization at the beginning of the block, if the logic flow only sometimes makes assignment to the variable, is perfectly fine, e.g.

	const char *string = NULL;

	if (...) {
        	string = ...
	}

But I would wish people stop doing this:

	const char *string = strchrnul(name, ':');

	... the real logic of the block that uses string follows ...

and instead say

	const char *string;

	string = strchrnul(name, ':');
	... the real logic of the block that uses string follows ...