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