Re: [PATCH 2/5] roaring.[ch]: apply Git specific changes to the roaring API

[Derrick Stolee <derrickstolee@github.com>]

On 9/19/2022 6:02 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>>>  int32_t array_container_write(const array_container_t *container, char *buf);
>>> +
>>> +int array_container_network_write(const array_container_t *container,
>>> +				  int (*write_fn) (void *, const void *, size_t),
>>> +				  void *data);
>>
>> Should we make write_fn a defined type? I'm not sure I've seen this
>> implicit type within a function declaration before.
> 
> Unless we can point out why having a named type is a good idea
> (e.g. we add such a function pointer as a member of a struct, or we
> keep a variable of that type somewhere), I actually would prefer to
> do without them.
> 
> Perhaps there are some more important reasons I am missing why we
> often come up with explicit types for callback function pointers in
> many parts of our API, but if there aren't, my preference actually
> is to lose them, not add more of them.
> 
> Hmph.... could "a typedef can become a place to give definitive
> documentation for the class of callback functions" be a good reason
> why we would want one?  I dunno.
> 
> In the posted patch, readers cannot tell what kind of three
> parameters they are supposed to give to write_fn().

This is exactly my reasoning. Having a clear definition gives us an
opportunity to document what each parameter is for, even if it is
just a variable name.

This anonymous type is used in multiple places, so it can be helpful
to know that the type is connected across call sites or a stack of
method calls.

In the unlikely event that we needed to modify this callback
signature, changing it in one place makes it clear that we cover
all connected uses instead of tracking all of these anonymous
functions across multiple methods.

Thanks,
-Stolee