On Mon, Oct 14, 2024 at 06:32:07PM -0400, Taylor Blau wrote: > On Mon, Oct 14, 2024 at 03:02:21PM +0200, Patrick Steinhardt wrote: > > We're about to introduce our own `reftable_buf` type to replace > > `strbuf`. Get rid of the seldomly-used `strbuf_addf()` function such > > that we have to reimplement one less function. > > Hmm. I count twelve calls to strbuf_addf() here in this patch that were > rewritten in terms of snprintf()ing to a temporary buffer. So I am not > sure that I agree that it is "seldomly-used". > > Sure, implementing fewer functions is nice, but I am not sure that > forcing the caller to use snprintf() directly is necessarily a > worthwhile trade-off. Another problem here is that snprintf() isn't exactly the most portable interface. Some systems don't have it at all, others have broken return code handling. So avoiding it completely makes that issue go away entirely. I can add that to the commit message if this needs a reroll. > Part of me wishes that we didn't have to write our own `reftable_buf` in > the first place. Could we use `strbuf` as-is and expose it through a > generic reftable-specific interface that users of reftable fill in with > a vtable or something? I tried that, and it felt way worse. The amount of code you have to write is roughly in the same ballpark, you don't have pluggable allocators, you don't have allocation error handling and every consumer would have to implement their own type. So overall it's only losses from my point of view. Patrick
