Re: [PATCH] Optimize common pattern of alloc_ref from string

From: Jeff King <peff@peff.net>
Date: May 10, 2008, 23:39 UTC
Message-ID: <20080510233918.GA315@sigill.intra.peff.net>
In-Reply-To: <1210462018-47060-1-git-send-email-kkowalczyk@gmail.com>

On Sat, May 10, 2008 at 04:26:58PM -0700, kkowalczyk@gmail.com wrote:

> As a byproduct, fixes one place where string wasn't properly terminated.

Great. Does this fix a user-visible bug? It would be nice to mention in the commit log _which_ place (though after reading the patch carefully, it looks like the one interpret_target) so that people looking at the commit later can understand exactly what was fixed.

> -	ref = alloc_ref(strlen(refname) + 1);
> -	strcpy(ref->name, refname);
> +	ref = alloc_ref_from_str(refname);

So this turns a 2-line construct into a 1-line construct...

Show 11 quoted lines

> +struct ref *alloc_ref_from_str(const char* str)
> +{
> +	struct ref *ret;
> +	unsigned len = strlen(str) + 1;
> +	char *tmp = xmalloc(sizeof(struct ref) + len);
> +	ret = (struct ref*)tmp;
> +	memset(tmp, 0, sizeof(struct ref));
> +	tmp += sizeof(struct ref);
> +	memcpy(tmp, str, len);
> +	return ret;
> +}

But why do we need an 8-line function to do it?

The only difference I can see over

  struct ref *alloc_ref_from_str(const char *str)
  {
    unsigned len = strlen(str) + 1;
    struct ref *ret = alloc_ref(len);
    memcpy(ret->name, str, len);
    return ret;
  }

is that we avoid memsetting the name portion of the struct to 0 before copying to it. It seems like an unproven micro-optimization that makes it a bit harder to read.

-Peff