Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Timur Safin" <tsafin@tarantool.org>
To: 'Alexander Turenko' <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers
Date: Tue, 13 Oct 2020 22:02:52 +0300	[thread overview]
Message-ID: <1ff701d6a193$73c5b6d0$5b512470$@tarantool.org> (raw)
In-Reply-To: <20201013182145.stzjxwv3zmodm6sd@tkn_work_nb>

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X v3] module api: box_ibuf_* wrappers
: 
: >  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].

Worth to note that version I've shown in Zoom did have ibuf.[hc]
but I've got an impression that Vlad had some complain against
it. 

: 
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +	if (!ibuf)
: > +		return;
: 
: I would assert on this.

Will add - good point.

: 
: > +	if (rpos)
: > +		*rpos = &ibuf->rpos;
: > +	if (wpos)
: > +		*wpos = &ibuf->wpos;
: 
: We use explicit == 0 / == NULL checks across our code.

It's already so in the branch - didn't send update.

: 
: > 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?

Yeah, good point - we are introducing it here just to avoid to have 
explicit dependency on it in header.

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

Yeah, that was introduced here at the moment I didn't return 
any value :)

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

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

The problem with the current API we have exposed - it's not 
self-contained, you could not do much with it, without calling 
other (internal yet) ibuf_* calls. That's why I essentially have 
copied here unit test from small but checked consistency of small
calls with results we could retrieve using newer api.

Regards,
Timur

  reply	other threads:[~2020-10-13 19:02 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12  0:44 [Tarantool-patches] [PATCH 2.X v3 0/3] module api: extend for external merger Lua module Timur Safin
2020-10-12  0:44 ` [Tarantool-patches] [PATCH 2.X v3 1/3] module api: export box_tuple_validate Timur Safin
2020-10-13  0:14   ` Alexander Turenko
2020-10-13  0:35     ` Timur Safin
2020-10-12  0:44 ` [Tarantool-patches] [PATCH 2.X v3 2/3] module api: export box_key_def_dup Timur Safin
2020-10-13  0:46   ` Alexander Turenko
2020-10-12  0:44 ` [Tarantool-patches] [PATCH 2.X v3 3/3] module api: luaL_checkibuf Timur Safin
2020-10-13 11:47   ` Alexander Turenko
2020-10-13 19:26     ` Igor Munkin
2020-10-13 16:30 ` [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers Timur Safin
2020-10-13 18:21   ` Alexander Turenko
2020-10-13 19:02     ` Timur Safin [this message]
2020-10-13 19:58       ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='1ff701d6a193$73c5b6d0$5b512470$@tarantool.org' \
    --to=tsafin@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox