On 4/6/2021 11:23 AM, Eric Sunshine wrote:
> On Tue, Apr 6, 2021 at 7:21 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>> I was not intending to make this re-entrant/thread safe. The intention
>> was to make it easy to consume the formatted string into output such
>> as a printf without needing to store a temporary 'char *' and free() it
>> afterwards. This ensures that the only lost memory over the life of the
>> process is at most one buffer. At minimum, these are things that could
>> be part of the message to justify this design.
>
> This has the failing that it won't work if someone calls it twice in
> the same printf() or calls it again before even consuming the first
> returned value, so this fails:
>
> printf("foo: %s\nbar: %s\n",
> refspec_item_format(...),
> refspec_item_format(...));
>
> as does this:
>
> const char *a = refspec_item_format(...);
> const char *b = refspec_item_format(...);
>
> Historically this project would "work around" that problem by using
> rotating static buffers in the function, but we've mostly been moving
> away from that for several reasons (can't predict how many buffers
> will be needed, re-entrancy, etc.).
>
>> So, I'm torn. This seems like a case where there is value in having
>> the return buffer be "owned" by this method, and the expected
>> consumers will use the buffer before calling it again. I'm not sure
>> how important it is to do this the other way.
>
> If history is any indication, we'd probably end up moving away from
> such an API eventually anyhow.
>
>> Would it be sufficient to justify this choice in the commit message
>> and comment about it in the method declaration? Or is it worth adding
>> this templating around every caller:
>>
>> char *buf = refspec_item_format(rsi);
>> ...
>> <use 'buf'>
>> ...
>> free(buf);
>
> An alternative would be to have the caller pass in a strbuf to be
> populated by the function. It doesn't reduce the boilerplate needed by
> the caller (still need to create and release the strbuf), but may
> avoid some memory allocations. But if this isn't a critical path and
> won't likely ever be, then passing in strbuf may be overkill.
>
>> I don't need much convincing to do this, but I hadn't properly
>> described my opinion before. Just a small nudge would convince me to
>> do it this way.
>
> For the reasons described above and earlier in the thread, avoiding
> the static buffer seems the best course of action.
OK, convinced. I'll return a string that must be freed in my
next version. Thanks!
-Stolee
