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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 31, 2016, 06:21 UTC
Message-ID: <alpine.DEB.2.20.1605310803580.4449@virtualbox>
In-Reply-To: <953965621.202433.1464614453377.JavaMail.zimbra@ensimag.grenoble-inp.fr>

Hi William,

On Mon, 30 May 2016, William Duclot wrote:

Show 10 quoted lines

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

Show 5 quoted lines

> >> +#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?

Show 16 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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