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

[Junio C Hamano <gitster@pobox.com>]

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

> The API contract is still respected:
>
> - The API users may peek strbuf.buf in-place until they perform an
>   operation that makes it longer (at which point the .buf pointer
>   may point at a new piece of memory).

I think the contract is actually a bit stronger; the API reserves
the right to free and reallocate a smaller chunk of memory if you
make the string shorter, so peeked value of .buf will not be relied
upon even if you didn't make it longer.

> - The API users may strbuf_detach() to obtain a piece of memory that
>   belongs to them (at which point the strbuf becomes empty), hence
>   needs to be freed by the callers.

Shouldn't you be honuoring another API contract?

 - If you allow an instance of strbuf go out of scope without taking
   the ownership of the string by calling strbuf_detach(), you must
   release the resource by calling strbuf_release().

As long as your "on stack strbuf" allows lengthening the string
beyond the initial allocation (i.e. .alloc, not .len), the user of
the API (i.e. the one that placed the strbuf on its stack) would not
know when the implementation (i.e. the code in this patch) decides
to switch to allocated memory, so it must call strbuf_release()
before it leaves.  Which in turn means that your implementation of
strbuf_release() must be prepared to be take a strbuf that still has
its string on the stack.

On the other hand, if your "on stack strbuf" does not allow
lengthening, I'd find such a "feature" pretty much useless.  The
caller must always test the remaining capacity, and switch to a
dynamic strbuf, which is something the caller would expect the API
implementation to handle silently.  You obviously do not have to
release the resource in such a case, but that is being convenient
in the wrong part of the API.

It would be wonderful if I can do:

	void func (void)
        {
		extern void use(char *[2]);
		/*
                 * strbuf that uses 1024-byte on-stack buffer
                 * initially, but it may be extended dynamically.
                 */
		struct strbuf buf = STRBUF_INIT_ON_STACK(1024);
		char *x[2];

		strbuf_add(&buf, ...); /* add a lot of stuff */
                x[0] = strbuf_detach(&buf, NULL);
		strbuf_add(&buf, ...); /* do some stuff */
                x[1] = strbuf_detach(&buf, NULL);
		use(x);

                strbuf_release(&buf);
	}

and add more than 2kb with the first add (hence causing buf to
switch to dynamic scheme), the first _detach() gives the malloc()ed 
piece of memory to x[0] _and_ points buf.buf back to the on-stack
buffer (and buf.alloc back to 1024) while setting buf.len to 0,
so that the second _add() can still work purely on stack as long as
it does not go beyond the 1024-byte on-stack buffer.

> +/**
> + * Flags
> + * --------------
> + */
> +#define STRBUF_OWNS_MEMORY 1
> +#define STRBUF_FIXED_MEMORY (1 << 1)

This is somewhat a strange way to spell two flag bits.  Either spell
them as 1 and 2 (perhaps in octal or hexadecimal), or spell them as
1 shifted by 0 and 1 to the left.  Don't mix the notation.

> @@ -20,16 +28,37 @@ char strbuf_slopbuf[1];
>  
>  void strbuf_init(struct strbuf *sb, size_t hint)
>  {
> +	sb->flags = 0;
>  	sb->alloc = sb->len = 0;
>  	sb->buf = strbuf_slopbuf;
>  	if (hint)
>  		strbuf_grow(sb, hint);
>  }
>  
> +void strbuf_wrap_preallocated(struct strbuf *sb, char *path_buf,
> +			      size_t path_buf_len, size_t alloc_len)
> +{
> +	if (!path_buf)
> +		die("you try to use a NULL buffer to initialize a strbuf");

What does "path" mean in the context of this function (and its
"fixed" sibling)?