Re: [PATCH v8 18/19] branch: use ref-filter printing APIs

From: Jeff King <peff@peff.net>
Date: Dec 12, 2016, 16:40 UTC
Message-ID: <20161212164020.btrrg2f62haxt7sh@sigill.intra.peff.net>
In-Reply-To: <CAOLa=ZT1KN_iw7JzB78hPb-LrY_yaDZk0D+TqY2W9hCOftzumA@mail.gmail.com>

On Mon, Dec 12, 2016 at 09:59:49PM +0530, Karthik Nayak wrote:

Show 19 quoted lines

> >> > This caller never stores the return value, and it ends up leaking. So I
> >> > wonder if you wanted "static struct strbuf" in the first place (and that
> >> > would explain the strbuf_reset() in your function).
> >>
> >> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
> >> suggestion.
> >>
> >> strbuf_detach() is also a better way to go.
> >
> > One of the other, though. If it's static, then you _don't_ want to
> > detach.
> >
> 
> Wait. Why not? since it gets added to the strbuf's within
> build_format() and that
> value is not needed again, why do we need to re-allocate? we can use the same
> variable again (i.e by keeping it as static).
> 
> I'm sorry, but I didn't get why these two should be mutually exclusive?

What is the memory ownership convention for the return value from the function?

If the caller should own the memory, then you want to do this:

  char *foo(...)
  {
	struct strbuf buf = STRBUF_INIT;
	... fill up buf ...
	return strbuf_detach(&buf, NULL);
  }

The "detach" disconnects the memory from the strbuf (which is going out of scope anyway), and the only pointer left to it is in the caller. It's important to use "detach" here and not just return the pointer, because that ensures that the returned value is always allocated memory (and never strbuf_slopbuf).

If the caller should not own the memory, then the function retains ownership. And you want something like this:

  char *foo(...)
  {
	static struct strbuf buf = STRBUF_INIT;
	strbuf_reset(&buf);
	... fill up buf ...
	return buf.buf;
  }

The same buffer is reused over and over. The "reset" call clears any leftover bits from the last invocation, and you must _not_ call strbuf_detach() in the return, as it disconnects the memory from the strbuf (and so next time you'd end up allocating again, and each return value becomes a memory leak).

Does that make sense?

-Peff