On Mon, Dec 12, 2016 at 10:10 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Mon, Dec 12, 2016 at 09:59:49PM +0530, Karthik Nayak wrote:
>
>> >> > 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).
Ah! perfect, makes perfect sense. Sorry you had to spell that out for me.
'strbuf_detach() in the return, as it disconnects the memory from the strbuf'
that was what I was missing, thanks.
--
Regards,
Karthik Nayak