On 26/08/14 12:03, Jeff King wrote:
[snip]
>
> Yeah, reading my suggestion again, it should clearly be
> alloc_flex_struct or something.
>
> Here's a fully-converted sample spot, where I think there's a slight
> benefit:
>
> diff --git a/remote.c b/remote.c
> index 3d6c86a..ba32d40 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -928,14 +928,30 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
> return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
> }
>
> +static void *alloc_flex_struct(size_t base, size_t offset, const char *fmt, ...)
> +{
> + va_list ap;
> + size_t extra;
> + char *ret;
> +
> + va_start(ap, fmt);
> + extra = vsnprintf(NULL, 0, fmt, ap);
> + extra++; /* for NUL terminator */
> + va_end(ap);
> +
> + ret = xcalloc(1, base + extra);
> + va_start(ap, fmt);
> + vsnprintf(ret + offset, extra, fmt, ap);
What is the relationship between 'base' and 'offset'?
Let me assume that base is always, depending on your compiler, either
equal to offset or offset+1. Yes? (I'm assuming base is always the
sizeof(struct whatever)). Do you need both base and offset?
> + va_end(ap);
> +
> + return ret;
> +}
> +
> static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
> const char *name)
> {
> - size_t len = strlen(name);
> - struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
> - memcpy(ref->name, prefix, prefixlen);
> - memcpy(ref->name + prefixlen, name, len);
> - return ref;
> + return alloc_flex_struct(sizeof(struct ref), offsetof(struct ref, name),
> + "%.*s%s", prefixlen, prefix, name);
> }
>
> struct ref *alloc_ref(const char *name)
>
> Obviously the helper is much longer than the code it is replacing, but
> it would be used in multiple spots. The main thing I like here is that
> we are dropping the manual length computations, which are easy to get
> wrong (it's easy to forget a +1 for a NUL terminator, etc).
>
> The offsetof is a little ugly. And the fact that we have a pre-computed
> length for prefixlen makes the format string a little ugly.
>
> Here's a another example:
>
> diff --git a/attr.c b/attr.c
> index 734222d..100c423 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -89,8 +89,8 @@ static struct git_attr *git_attr_internal(const char *name, int len)
> if (invalid_attr_name(name, len))
> return NULL;
>
> - a = xmalloc(sizeof(*a) + len + 1);
> - memcpy(a->name, name, len);
> + a = alloc_flex_array(sizeof(*a), offsetof(struct git_attr, name),
> + "%.*s", len, name);
> a->name[len] = 0;
> a->h = hval;
> a->next = git_attr_hash[pos];
>
> I think this is strictly worse for reading. The original computation was
> pretty easy in the first place, so we are not getting much benefit
> there. And again we have the precomputed "len" passed into the function,
> so we have to use the less readable "%.*s". And note that offsetof()
> requires us to pass a real typename instead of just "*a", as sizeof()
> allows (I suspect passing "a->name - a" would work on many systems, but
> I also suspect that is a gross violation of the C standard when "a" has
> not yet been initialized).
>
> So given that the benefit ranges from "a little" to "negative" in these
> two examples, I'm inclined to give up this line of inquiry.
Indeed. :-D
ATB,
Ramsay Jones
--
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