From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id D72C2469719
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 13 Oct 2020 22:58:10 +0300 (MSK)
Date: Tue, 13 Oct 2020 22:58:29 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201013195829.mrecicqofwk5j36z@tkn_work_nb>
References: <cover.1602463103.git.tsafin@tarantool.org>
 <0cc5173291c618e58934b0460256145f42c64444.1602606208.git.tsafin@tarantool.org>
 <20201013182145.stzjxwv3zmodm6sd@tkn_work_nb>
 <1ff701d6a193$73c5b6d0$5b512470$@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1ff701d6a193$73c5b6d0$5b512470$@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_*
	wrappers
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Timur Safin <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

> : > +
> : > +#include <trivia/util.h>
> : > +#include <small/ibuf.h>
> : 
> : How about just define an opaque structure?
> 
> Yeah, good point - we are introducing it here just to avoid to have 
> explicit dependency on it in header.

Hm. It is out of /* \cond public */ and /* \endcond public */ markers,
so it's fine from this point of view. But using of `struct ibuf;` in the
header instead of `#include <small/ibuf.h>` should decrease the compile
time a bit. In fact, the time difference is negligible, so the point is
more regarding rules of thumb and feeling about the code as 'clean'.

> 
> : 
> :  | struct ibuf;
> : 
> : But include small/ibuf.h explicitly in the .c file?
> : 

(Cited for convenience.)

> : > +	struct slab_cache *slabc = cord_slab_cache();
> : > +	assert(slabc != NULL);
> : > +	box_ibuf_t ibuf;
> : > +
> : > +	ibuf_create(&ibuf, slabc, 16320);
> : > +	assert(ibuf_used(&ibuf) == 0);
> : > +	box_ibuf_reserve(&ibuf, 65536);
> : > +	char **rpos;
> : > +	char **wpos;
> : > +	box_ibuf_read_range(&ibuf, &rpos, &wpos);
> : > +
> : > +	void *ptr = ibuf_alloc(&ibuf, 10);
> : > +	assert(ptr != NULL);
> : > +
> : > +	assert(ibuf_used(&ibuf) == 10);
> : > +	assert((*wpos - *rpos) == 10);
> : 
> : Now box_ibuf_read_range() should give the updated wpos.
> 
> Here I didn't get your point - wpos and rpos are pointers 
> to fields inside of ibuf structure, they will not change 
> regardless the number of calls to we perform. 

Let's show a sketch:

 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | <...do some allocations...>
 | assert((*wpos - *rpos) == 10);

This test case verifies that the positions we obtained before the
allocations are valid after them. I proposed to also verify that
box_ibuf_read_range() will correctly return the positions after the
allocations:

 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | <...do some allocations...>
 | assert((*wpos - *rpos) == 10);
 |
 | /* Renew positions and verify them. */
 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | assert((*wpos - *rpos) == 10);

> 
> : 
> : > +
> : > +	ptr = ibuf_alloc(&ibuf, 10000);
> : > +	assert(ptr);
> : > +	assert(ibuf_used(&ibuf) == 10010);
> : > +	assert((*wpos - *rpos) == 10010);
> : 
> : Same here.

(Cited for convenience.)