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

From: William Duclot <william.duclot@ensimag.grenoble-inp.fr>
Date: Jun 3, 2016, 14:04 UTC
Message-ID: <20160603140444.GA3903@Messiaen>
In-Reply-To: <574D0D99.6080303@alum.mit.edu>

On Tue, May 31, 2016 at 06:05:45AM +0200, Michael Haggerty wrote:

Show 22 quoted lines

> When reading this patch series, I found I had trouble remembering
> whether "preallocated" meant "preallocated and movable" or "preallocated
> and immovable". So maybe we should brainstorm alternatives to
> "preallocated" and "fixed". For example,
> 
> * "growable"/"fixed"? Seems OK, though all strbufs are growable at least
> to the size of their initial allocation, so maybe "growable" is misleading.
> 
> * "movable"/"fixed"? This maybe better captures the essence of the
> distinction. I'll use those names below for concreteness, without
> claiming that they are the best.
> 
> [...]
> 
>                                                 The functions might be
> named more like `strbuf_attach()` to emphasize their similarity to that
> existing function. Maybe
> 
>     strbuf_attach_fixed(struct strbuf *sb, void *s, size_t len, size_t
> alloc);
>     strbuf_attach_movable(struct strbuf *sb, void *s, size_t len, size_t
> alloc);

Now that I am looking in detail into it, I am not so convinced by those
names. Using "attach" suggests the same behavior as strbuf_attach(),
which is _not_ the case to me:
    - The aim of the attach() function is to give ownership of a
      buffer allocated by the caller to the strbuf.
    - The aim of the wrap() functions is to give the right to use a
      buffer allocated by the caller to the strbuf, while keeping
      ownership.
    - For completion: the aim of the init() function is to let the
      strbuf manage its own buffer.

I think that it is important to distinct those 3 use cases for the API user to be able to understand. And to describe this API extension, "wrap" seems clear to me. Another point that would makes me skeptical about using `strbuf_attach_preallocated()` is that the real difference with `strbuf_attach()` isn't in the allocation of the buffer parameter, it is in the ownership. Both takes a buffer allocated by the user as parameter (so a preallocated buffer), even thought `strbuf_attach_preallocated()` may use a stack-allocated one.

So I come to the conclusion that even using the word "preallocated" may not be such a good idea, as even strbuf_attach() use a preallocated buffer. What I think would be the clearer would be:

    - strbuf_attach()       (unchanged)
    - strbuf_wrap()         (no need for the "preallocated")
    - strbuf_wrap_fixed()
    - STRBUF_WRAP   
    - STRBUF_WRAP_FIXED

The two last ones would be macros, equivalent to the functions except that they don't release the strbuf before initializing it.

What do you think about this?