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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi William,

On Mon, 30 May 2016, William Duclot wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > 	When working e.g. with file paths or with dates, strbuf's
> > 	malloc()/free() dance of strbufs can be easily avoided: as
> > 	a sensible initial buffer size is already known, it can be
> > 	allocated on the heap.
> 
> strbuf already allow to indicate a sensible initial buffer size thanks
> to strbuf_init() second parameter. The main perk of pre-allocation
> is to use stack-allocated memory, and not heap-allocated :)

Sorry, my brain thought about the next point while my fingers typed
"heap". It is "stack" I meant.

> >> +#include <sys/param.h>
> > 
> > Why?
> 
> For the MAX macro. It may be a teeny tiny overkill

Teeny tiny.

You probably assumed that everybody compiles this on Linux, and since it
compiles on your machine, no problem, right?

> >> @@ -38,7 +67,11 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
> >>  {
> >>  	char *res;
> >>  	strbuf_grow(sb, 0);
> >> -	res = sb->buf;
> >> +	if (sb->flags & STRBUF_OWNS_MEMORY)
> >> +		res = sb->buf;
> >> +	else
> >> +		res = xmemdupz(sb->buf, sb->alloc - 1);
> > 
> > This looks like a usage to be avoided: if we plan to detach the buffer,
> > anyway, there is no good reason to allocate it on the heap first. I would
> > at least issue a warning here.
> 
> strbuf_detach() guarantees to return heap-allocated memory, that the caller
> can use however he want and that he'll have to free.

First of all, let's stop this "caller == he" thing right here and now. A
better way is to use the "singular they": ... the caller can use however
they want...

> If the strbuf doesn't own the memory, it cannot return the buf attribute
> directly because: [...]

I know that. My point was that it is more elegant to allocate the buffer on
the heap right away, if we already know that we want to detach it.

I'll reply to Michael's comment on this.

> - The memory belong to someone else (so the caller can't use it however
> he want)

s/belong/belongs/ (just because the 's' is often silent in spoken French
does not mean that you can drop it from written English)

> >>  extern char strbuf_slopbuf[];
> >> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> >> +#define STRBUF_INIT  { 0, 0, 0, strbuf_slopbuf }
> > 
> > If I am not mistaken, to preserve the existing behavior the initial flags
> > should be 1 (own memory).
> 
> strbuf_slopbuf is a buffer that doesn't belong to any strbuf (because it's
> shared between all just-initialized strbufs).

Yet we somehow already have handling for that, right? Makes me wonder why
I did not see that handling converted to the STRBUF_OWNS_MEMORY way...

> > BTW this demonstrates that it may not be a good idea to declare the
> > "flags" field globally but then make the actual flags private.
> 
> I'm not sure what you mean here?

What I meant is that the global _INIT cannot set any flags, because the
constants are not available globally. And that seems odd. If you ever want
to introduce something like STRBUF_INIT_WRAP(buffer), you would *require*
those flag constants to be public.

> > Also: similar use cases in Git used :1 flags (see e.g. the "configured"
> > field in credential.h).
> 
> I think that keeping an obscure `flags` attribute may be better, as they
> should only be useful for internal operations and the user shouldn't mess
> with it. Keeping it a `private` attribute, in a way

This is not C++. To do what you intend to do, you would require C++ style
visibility scopes, where a constructor can use a private enum. If you
truly want to go this way, I fear you will have *a lot more* to do than
this 2-patch series: there are a *ton* of existing header files
disagreeing with this goal.

Ciao,
Johannes