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

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

On Tue, May 31, 2016 at 06:05:45AM +0200, Michael Haggerty wrote:
> On 05/30/2016 02:52 PM, Matthieu Moy wrote:
> > [...]
> 
> I feel bad bikeshedding about names, especially since you took some of
> the original names from my RFC. But names are very important, so I think
> it's worth considering whether the current names could be improved upon.
> 
> 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.

Yes, the names are debatable

> > * 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.
> 
> Hmmm....
> 
> I think the best way to answer these questions is to think about use
> cases and make them as easy/consistent as possible.
> 
> I expect that a very common use of strbuf_wrap_fixed() will be to wrap a
> stack-allocated string, like
> 
>     char pathbuf[PATH_MAX];
>     struct strbuf path;
> 
>     strbuf_wrap_fixed(&path, pathbuf, 0, sizeof(pathbuf));
> 
> In this use case, it would be a shame if `path` had to be initialized to
> STRBUF_INIT just because `strbuf_wrap_fixed()` calls `strbuf_release()`
> internally.
> 
> But maybe we could make this use case easier still. If there were a macro
> 
>     #define STRBUF_FIXED_WRAPPER(sb, buf, len) { STRBUF_FIXED_MEMORY,
> sizeof(buf), (len), (buf) }
> 
> then we could write
> 
>     char pathbuf[PATH_MAX];
>     struct strbuf path = STRBUF_FIXED_WRAPPER(pathbuf, 0);
> 
> I think that would be pretty usable. One would have to be careful only
> to wrap arrays and not `char *` pointers, because `sizeof` wouldn't work
> on the latter. The BARF_UNLESS_AN_ARRAY macro could be used here to add
> a little safety.

That sounds like a good idea to me

> [...]
> If you provide macro forms like these for initializing strbufs, then I
> agree with Matthieu that the analogous functional forms should probably
> call strbuf_release() before wrapping the array. 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);

I'm not a big fan of the idea that the macro and the function don't have
the same behavior. Using "attach" instead of "wrap" or "init" may
justify that