Mike Hommey <mh@xxxxxxxxxxxx> writes:
> 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.
-
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
