Hi William,

On Mon, 30 May 2016, William Duclot wrote:

Johannes Schindelin [off-list ref] writes:

quoted

	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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

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