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

[Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>]

On Tue, Sep 20, 2022 at 5:49 PM Derrick Stolee <derrickstolee@github.com> wrote:
>
> 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.

Agreed.

> 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.

Got it. Thanks!