Hi Michael,
On Tue, 31 May 2016, Michael Haggerty wrote:
> On 05/30/2016 02:13 PM, Johannes Schindelin wrote:
> > [...]
> >> @@ -38,7 +67,11 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
> >> {
> >> char *res;
> >> strbuf_grow(sb, 0);
> >> - res = sb->buf;
> >> + if (sb->flags & STRBUF_OWNS_MEMORY)
> >> + res = sb->buf;
> >> + else
> >> + res = xmemdupz(sb->buf, sb->alloc - 1);
> >
> > This looks like a usage to be avoided: if we plan to detach the buffer,
> > anyway, there is no good reason to allocate it on the heap first. I would
> > at least issue a warning here.
>
> I think this last case should be changed to
>
> res = xmemdupz(sb->buf, sb->len);
>
> Johannes, if this change is made then I think that there is a reasonable
> use case for calling `strbuf_detach()` on a strbuf that wraps a
> stack-allocated string, so I don't think that a warning is needed.
>
> I think this change makes sense. After all, once a caller detaches a
> string, it is probably not planning on growing/shrinking it anymore, so
> any more space than that would probably be wasted. In fact, since the
> caller has no way to ask how much extra space the detached string has
> allocated, it is almost guaranteed that the space would be wasted.
Ah, I did not think of that. (We could of course introduce
strbuf_realloc_snugly() or some such, but I agree: why?) So the best
counter-example to my objection might be something like this:
-- snip --
Subject: git_pathdup(): avoid realloc()
When generating Git paths, we already know the maximum length:
MAX_PATH. Let's avoid realloc()-caused fragmentation by using a
fixed buffer.
As a side effect, we avoid overallocating the end result, too:
previously, strbuf_detach() would not realloc() to avoid wasting
the (sb->alloc - sb->len) bytes, while it malloc()s the precisely
needed amount of memory when detaching fixed buffers.
diff --git a/path.c b/path.c
index 2511d8a..64fd3ee 100644
--- a/path.c
+++ b/path.c
@@ -426,7 +426,8 @@ const char *git_path(const char *fmt, ...)
char *git_pathdup(const char *fmt, ...)
{
- struct strbuf path = STRBUF_INIT;
+ char buffer[MAX_PATH];
+ struct strbuf path = STRBUF_WRAP_FIXED(buffer);
va_list args;
va_start(args, fmt);
do_git_path(&path, fmt, args);
-- snap --
Color me convinced,
Dscho
--
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