Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Fri, Oct 16, 2020 at 12:42:26AM +0300, Alexander Turenko wrote:
> On Fri, Oct 16, 2020 at 12:35:05AM +0300, Timur Safin wrote:
> > After our discussions within messenger, I've updated ibuf.c to
> > raise error if there is invalid argument passed, to follow
> > current box_* functions convention. E.g.
> 
> Nit: it *sets* an error, not raise.
> 
> > 
> > +void
> > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> > +{
> > +       if (ibuf == NULL) {
> > +               diag_set(ClientError, ER_ILLEGAL_PARAMS,
> > +                        "Invalid ibuf argument");
> > +               return;
> > +       }
> > +       if (rpos != NULL)
> > +               *rpos = &ibuf->rpos;
> > +       if (wpos != NULL)
> > +               *wpos = &ibuf->wpos;
> > +}
> 
> You should return a code (0 / -1) so. The diagnostics area is like
> errno: a caller should lean on the code, not the diagnostics area to
> determine whether an error took place. If it occurs, then the
> diagnostics is guaranteed to exist and to be actual.

Hey, stop. Is there any other function in the module API that allows to
pass NULL instead of an object? Is there a valid use case for this?
Should a user check box_ibuf_read_range() return value and handle a
possible error?

I don't see any sense in handling NULL here. Let's leave assert(ibuf !=
NULL) or nothing.

The practice about silengly ignore incorrect input parameters that you
mentioned before is certainly bad patters. I suggest to avoid it.