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

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

> : > +}
> : > +
> : > +void
> : > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> : > +{
> : > +	assert(ibuf != NULL);
> : > +	if (ibuf == NULL)
> : > +		return;
> : > +	if (rpos != NULL)
> : > +		*rpos = &ibuf->rpos;
> : > +	if (wpos != NULL)
> : > +		*wpos = &ibuf->wpos;
> : 
> : 3. I would better assume all the arguments are not NULL, and document
> : it. Especially ibuf itself. We need some border where to stop the
> : sanity checks, and this looks like an overkill already. For example,
> : box_tuple_ref() also works with a pointer, but it does not check for
> : it being not NULL. It is just stupid to call method of an object with
> : that object passed as NULL. The same below.
> 
> Please see updated branch tsafin/gh-5273-expand-module-api-v4 for the 
> current version. For rpos and wpos I need to check for NULL because 
> we may call for only single, particular argument (like we do in some 
> cases in current external module) thus we pass NULL there.

I think we should allow to pass NULL to the third parameters, but not to
the second ones. I don't see a reason to obtain 'wpos' using
_read_range() (it looks more natural to do so using _write_range()) and
to obtain just 'end' without left side of the buffer. To copy something
from right to left without the left border check? Maybe... However it
does not look how the for 'ibuf', where both pointers are moving forward
(it is defined by the _reserve() semantic).