Re: [PATCH] speed: reuse char instead of recreation in loop

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: May 25, 2009, 20:39 UTC
Message-ID: <4A1B01E9.30601@lsrfire.ath.cx>
In-Reply-To: <pan.2009.05.25.19.44.10@fedoraproject.org>

Thomas Spura schrieb:

> Move a char and a char * outside of a for loop for speed improvements

It's a good idea to include actual timings, to give the reader a better idea what operation is sped up and by how much.

Show 13 quoted lines

> Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
> ---
> Comments?
> 
>  transport.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/transport.c b/transport.c
> index 17891d5..e350937 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf 
> *temp_dir,

Please turn off automatic line wrapping in your email program when sending patches (at least for the patch part).

Show 10 quoted lines

>  		int refspec_nr, const char **refspec)
>  {
>  	int i;
> +	unsigned char sha1[20];
> +	char *ref;
>  
>  	for (i = 0; i < refspec_nr; i++) {
> -		unsigned char sha1[20];
> -		char *ref;
> -

I wouldn't expect this to significantly change the object code. Declaring variables in as narrow a scope as possible often helps to make the code more readable, though. write_refs_to_temp_dir() is short enough, so it doesn't matter in this case, though.

Show 14 quoted lines

>  		if (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !
> = 1)
>  			return error("Could not get ref %s", refspec[i]);
>  
> @@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf 
> *temp_dir,
>  			free(ref);
>  			return -1;
>  		}
> -		free(ref);
>  	}
> +	free(ref);
>  	return 0;
>  }

This introduces a memory leak. The string pointed to by ref is allocated by dwim_ref() and needs to be free()'d after use, and -- more importantly -- before ref is assigned its next value by dwim_ref().

René