Re: [PATCH 2/2] strbuf: allow to use preallocated memory

William Duclot <william.duclot@ensimag.grenoble-inp.fr> writes:

prevent strbuf from being used ...

s/try/tries/

If you set flags = 0 here, existing callers will have all flags off, including OWNS_MEMORY.

I *think* this is OK, as sb->buf is currently pointing to strbuf_slopbuf, which the the strbuf doesn't own. But that is too subtle to go without an explanatory comment IMHO.

Also, doesn't this make the "new_buf" case useless in strbuf_grow?

With your patch, the code looks like:

void strbuf_grow(struct strbuf *sb, size_t extra)
{
	int new_buf = !sb->alloc;
...
	if (sb->flags & STRBUF_OWNS_MEMORY) {
		if (new_buf) // <---------------------------------------- (1)
			sb->buf = NULL;
		ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
	} else {
		/*
		 * The strbuf doesn't own the buffer: to avoid to realloc it,
		 * the strbuf needs to use a new buffer without freeing the old
		 */
		if (sb->len + extra + 1 > sb->alloc) {
			size_t new_alloc = MAX(sb->len + extra + 1, alloc_nr(sb->alloc));
			char *buf = xmalloc(new_alloc);
			memcpy(buf, sb->buf, sb->alloc);
			sb->buf = buf;
			sb->alloc = new_alloc;
			sb->flags |= STRBUF_OWNS_MEMORY;
		}
	}

	if (new_buf) // <---------------------------------------- (2)
		sb->buf[0] = '\0';
}

I think (1) is now dead code, since sb->alloc == 0 implies that STRBUF_OWNS_MEMORY is set. (2) seems redundant since you've just memcpy-ed the existing '\0' into the buffer.

strbuf_wrap_preallocated seem very close to strbuf_attach. I'd rather see a symmetric code sharing like

void strbuf_attach_internal(struct strbuf *sb, ..., unsigned int flags)

and then strbuf_attach() and strbuf_wrap_preallocated() become straightforward wrappers around it.

This would avoid setting and then unsetting STRBUF_OWNS_MEMORY (the performance impact is probably small, but it looks weird to set the flag and then unset it right away).

After your patch, there are differences between strbuf_wrap_preallocated() which I think are inconsistencies:

* strbuf_attach() does not check for NULL buffer, but it doesn't accept
  them either if I read correctly. It would make sense to add the check
  to strbuf_attach(), but it's weird to have the performance-critical
  oriented function do the expensive stuff that the
  non-performance-critical one doesn't.

* strbuf_attach() calls strbuf_release(), which allows reusing an
  existing strbuf. strbuf_wrap_preallocated() calls strbuf_init which
  would override silently any previous content. I think strbuf_attach()
  does the right thing here.

  (And I'm probably the one who misguided you to do this)

  In any case, you probably want to include calls to strbuf_attach() and
  strbuf_wrap_*() functions on existing non-empty strbufs.

And this could become a 3rd caller of strbuf_attach_internal().

I think you want to memcpy only sb->len + 1 bytes. Here, you're memcpy-ing the allocated-but-not-initialized part of the array.

xmemdupz can probably simplify the code too (either you include the '\0' in what memcpy copies, or you let xmemdupz add it).

To make it clearer: "a new buffer will then be allocated"?

Perhaps say explicitly that although the allocated buffer has a fixed size, the string itself can grow as long as it does not overflow the buffer?

Better than just warn about performance, you can give the alternative.