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

[Michael Haggerty <mhagger@alum.mit.edu>]

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.

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

(It would be even nicer if we could write

    struct strbuf path = STRBUF_FIXED(PATH_MAX);

and it would initialize both path and a pathbuf variable for it to wrap,
but I don't think there is a way to implement such a macro. So I think
the only way to squeeze this onto one line would be to make it look like

    DEFINE_FIXED_STRBUF(path, PATH_MAX);

But that looks awful, so I think the two-line version above is preferable.)

Similarly, there could be a macro

    #define STRBUF_MOVABLE_WRAPPER(sb, buf, len) { 0, sizeof(buf),
(len), (buf) }

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);

Michael