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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html