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

[William <william.duclot@ensimag.grenoble-inp.fr>]

On Mon, May 30, 2016 at 11:34:42PM -0700, Junio C Hamano wrote:
> 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.

Right, anytime the string size change
 
>> - 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.

Well, my implementation does handle a strbuf that still has its
string on the stack: the buffer won't be freed in this case (only a
reset to STRBUF_INIT).
Unless I misunderstood you?

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


I think that it's possible, but extends the API beyond what was
originally intended by Michael. I don't see other use cases than treating
several string sequentially, is it worth avoiding a attach_movable() 
(as suggested by Michael in place of wrap_preallocated())?
You could do:

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

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

Which seems pretty close without needing the strbuf API to remember the
original buffer or to even care about memory being on the stack or the
heap.

>> +/**
>> + * 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.

Noted

>> @@ -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)?

That should be someting like `str` and `str_len` indeed