From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104])
 (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 13974469719
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 13 Oct 2020 21:21:25 +0300 (MSK)
Date: Tue, 13 Oct 2020 21:21:45 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201013182145.stzjxwv3zmodm6sd@tkn_work_nb>
References: <cover.1602463103.git.tsafin@tarantool.org>
 <0cc5173291c618e58934b0460256145f42c64444.1602606208.git.tsafin@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <0cc5173291c618e58934b0460256145f42c64444.1602606208.git.tsafin@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

>  create mode 100644 src/box/box_ibuf.c
>  create mode 100644 src/box/box_ibuf.h

I would name it just src/box/ibuf.[ch].

> +void
> +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> +{
> +	if (!ibuf)
> +		return;

I would assert on this.

> +	if (rpos)
> +		*rpos = &ibuf->rpos;
> +	if (wpos)
> +		*wpos = &ibuf->wpos;

We use explicit == 0 / == NULL checks across our code.

> index 000000000..235b87560
> --- /dev/null
> +++ b/src/box/box_ibuf.h
> @@ -0,0 +1,68 @@
> +#pragma once
> +/*
> <...>
> +
> +#include <stddef.h>

It defines NULL and size_t as well as stdlib.h, but you include stdlib.h
in the .c file, so I would use the same header in both. Not sure which
one should be preferred.

> +
> +#include <trivia/util.h>
> +#include <small/ibuf.h>

How about just define an opaque structure?

 | struct ibuf;

But include small/ibuf.h explicitly in the .c file?

> +static int
> +test_box_ibuf(lua_State *L)
> +{
> +	(void)L;

There is usage of L at end of the function.

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

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

Same here.

> +
> +	ibuf_reset(&ibuf);
> +	assert(ibuf_used(&ibuf) == 0);

I would test box_ibuf_write_range() as well.

The test per se is more like how internal ibuf operations are
interoperate with the public API rathen than a unit test of the public
API. However it is okay, it'll also do the work and I would not bother
much now.