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

[Jeff King <peff@peff.net>]

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...

> +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