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

["Timur Safin" <tsafin@tarantool.org>]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_*
: wrappers
: 
: Thanks for the patch!
: 
: See 4 comments below.
: 
...
: > +
: > +#include <stdlib.h>
: > +
: > +#include "ibuf.h"
: > +#include <small/ibuf.h>
: 
: 1. Please, don't include non-system headers using <>. Use "".
: The same for box/ibuf.h.

Ok.

: 
: > +
: > +void *
: > +box_ibuf_reserve(box_ibuf_t *ibuf, size_t size)
: > +{
: > +	return ibuf_reserve(ibuf, size);
: 
: 2. It should set a diag error in case of fail.

This is already updated accordingly in the branch
tsafin/gh-5273-expand-module-api-v4.

: 
: > +}
: > +
: > +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. 


: 
: > +}
: > +
: > +void
: > +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
: > +{
: > +	if (ibuf == NULL)
: > +		return;
: > +	if (wpos != NULL)
: > +		*wpos = &ibuf->wpos;
: > +	if (end != NULL)
: > +		*end = &ibuf->end;
: > +
: > +}
...
: > +
: > +/**
: > + * Reserve requested amount of bytes in ibuf buffer
: 
: 4. Please, finish sentences with a dot. I don't understand.
: Is it so hard for people to put a dot in the end? Does it
: cost money or something? Do people write the same way in
: other places, like emails, documents? Why is the code allowed
: to be treated worse?
: 
: Talking of the comments content - they are mostly useless.
: What is read range, what is write range? How do they relate?
: Is a user supposed to update them manually, when work with the
: buffer? Current description makes it impossible to use
: box_ibuf_t without reading the source code. What users are
: not supposed to do usually to be able to use the API.

I'll return to comments and test in a follow-up patchset 
(see my earlier discussion with A.Turenko) and there I'll
put all dots. Promise! In very meaningful comments. Promise!!

Timur