Re: [PATCH] Simplify the code and avoid an attribution.

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2007, 20:29 UTC
Message-ID: <7vk5oa3tyg.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071122200157.GC19675@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 34 quoted lines

> On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:
>> --- a/config.c
>> +++ b/config.c
>> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
>> char *filename)
>>  	int ret;
>>  	FILE *f = fopen(filename, "r");
>> 
>> -	ret = -1;
>> -	if (f) {
>> -		config_file = f;
>> -		config_file_name = filename;
>> -		config_linenr = 1;
>> -		ret = git_parse_file(fn);
>> -		fclose(f);
>> -		config_file_name = NULL;
>> -	}
>> +	if (!f)
>> +		return -1;
>> +
>> +	config_file = f;
>> +	config_file_name = filename;
>> +	config_linenr = 1;
>> +	ret = git_parse_file(fn);
>> +	fclose(f);
>> +	config_file_name = NULL;
>> +
>>  	return ret;
>>  }
>
> Actually, since it is more likely that the file has been opened, the
> original code is more optimal because it doesn't generate a jump in most
> cases. And if you're worried about the ret variable, don't worry, it's
> most likely stripped out by the compiler optimizations.

I did not comment on this patch partly because I did not know what "attribution" meant. I think it is a good change from readability, not micro-optimization, point of view.

If you structure your code this way,

	do this
        if (there is an error)
        	return error code;
	do that
	do the rest

it is much easier to read than

	do this
        set error code pessimistically
        if (it was successful) {
        	do that
                update error code
                do the rest
	}
        return error code

especially the more likely part that runs the real processing ("do that...do the rest") tends to grow over time, and even acquire other error checks.