On Wed, Oct 16, 2024 at 04:56:52PM -0400, Taylor Blau wrote: > On Wed, Oct 16, 2024 at 10:42:44AM +0200, Patrick Steinhardt wrote: > > On Tue, Oct 15, 2024 at 03:27:29PM -0400, Taylor Blau wrote: > > > On Tue, Oct 15, 2024 at 01:10:59AM -0400, Eric Sunshine wrote: > > > > On Tue, Oct 15, 2024 at 12:38 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > > > > On Mon, Oct 14, 2024 at 06:34:55PM -0400, Taylor Blau wrote: > > > > > > On Mon, Oct 14, 2024 at 03:02:24PM +0200, Patrick Steinhardt wrote: > > > > > > > +/* > > > > > > > + * Add the given bytes to the buffer. Returns 0 on success, > > > > > > > + * REFTABLE_OUT_OF_MEMORY_ERROR on allocation failure. > > > > > > > + */ > > > > > > > +int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len); > > > > > > > > > > > > Is there a reason that data is a void-pointer here and not a const char > > > > > > *? > > > > > > > > > > Only that it emulates `strbuf_add()`, which also uses a void pointer. > > > > > > > > The reason for that is because strbuf is a generic byte-array which > > > > may contain embedded NULs, and the `const void *` plus `len` > > > > emphasizes this property, whereas `const char *` would imply a > > > > C-string with no embedded NULs. > > > > > > Thanks, that was the explanation I was missing. Perhaps it is worth > > > re-stating in the commit message here to avoid confusing readers like I > > > was when I first read Patrick's patch ;-). > > > > Does it make sense to explicitly state how the interfaces look like > > though? I don't do that for the other functions either, and for most of > > the part I just reuse the exact same function arguments as we had with > > the strbuf interface. > > I don't feel very strongly about it, but I had suggested it because my > initial read of this patch confused me, and I had wondered if others may > be similarly confused. > > For what it's worth, I was thinking something on the order of the > following added to the patch message: > > Note that the reftable_buf_add() function intentionally takes a "const > void *" instead of a "const char *" (as does its strbuf counterpart, > strbuf_add()) to emphasize that the buffer may contain NUL characters. > > But, as I said, I don't feel very strongly about it. You know: let me amend the function documentation itself. That feels way less out of place compared to putting this info into the commit message and has the benefit that a future reader of the code will know why we have types without digging into the commit history. Patrick
