From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 7378E469719 for ; Tue, 13 Oct 2020 22:02:58 +0300 (MSK) From: "Timur Safin" References: <0cc5173291c618e58934b0460256145f42c64444.1602606208.git.tsafin@tarantool.org> <20201013182145.stzjxwv3zmodm6sd@tkn_work_nb> In-Reply-To: <20201013182145.stzjxwv3zmodm6sd@tkn_work_nb> Date: Tue, 13 Oct 2020 22:02:52 +0300 Message-ID: <1ff701d6a193$73c5b6d0$5b512470$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Alexander Turenko' Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org : From: Alexander Turenko : 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 : : 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 : > +#include : : 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