From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9B2A36EC55; Thu, 26 Aug 2021 17:57:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9B2A36EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629989825; bh=l3reKF6nsc2wc0awuGff4kkvX9n5RKC1XQrUI15KB6w=; h=References:In-Reply-To:Date:To:Cc:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=jvY/lbMCLBWPEvtNA80qgReWWCm1mh7Oa8QfZ6xCc+fNOytsIS8TDCft0JFwuA8la XlUw8U6WyeRLZc3z88ZxuQ0aKnT9YmLDGZGl6maHir+PKG7tePMwNH2/BBwOy0nkuH hdsQQnU/QPZPJjQQreR59giykcgdlH4RMIqggqZk= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 14A0E6EC55 for ; Thu, 26 Aug 2021 17:57:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 14A0E6EC55 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1mJGoR-0000kH-FD for tarantool-patches@dev.tarantool.org; Thu, 26 Aug 2021 17:57:03 +0300 Received: by mail-lj1-f181.google.com with SMTP id l18so5614670lji.12 for ; Thu, 26 Aug 2021 07:57:03 -0700 (PDT) X-Gm-Message-State: AOAM531mKFZAkIjQWKyIU/AbNO08ASDcdoGz70ruGz1EB7GgbFjFgq5g UEMZzXBoFwDf5akkoNCA2K6FaavkTSplCDVoHQ== X-Google-Smtp-Source: ABdhPJwvaQV4STniU4z9uQUc4GmNZ5yS83S/cjiz1PkrftjCULljZThwGNz0VQWXNeVzaGx6kBJBzZ/nx10LWQeFV6s= X-Received: by 2002:a2e:9154:: with SMTP id q20mr3387843ljg.455.1629989823034; Thu, 26 Aug 2021 07:57:03 -0700 (PDT) MIME-Version: 1.0 References: <20210817075856.xkelwdonp5kmdofs@esperanza> <38b7f8367e68ece293394d9cf88200e7929341de.1629186968.git.vdavydov@tarantool.org> <726cc8fe-c43b-26a3-b810-da7dc9c295fc@tarantool.org> <20210825133504.mwquu2s7tzvcf7m2@esperanza> In-Reply-To: <20210825133504.mwquu2s7tzvcf7m2@esperanza> Date: Thu, 26 Aug 2021 17:56:46 +0300 X-Gmail-Original-Message-ID: Message-ID: To: Vladimir Davydov Cc: Vladislav Shpilevoy , Yaroslav Dynnikov , tml Content-Type: multipart/alternative; boundary="00000000000063bad905ca7794da" X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9736CF3E71F18CE0C3E1D5927724F4AAA182A05F538085040C9D253AB807FCE93A7CC4D1448FF8CCFAC618B68B916F4C170D040BFFC5165FC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A0175C48BD57B26BC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE71F31443D278F8A69EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2A8FA649659F1D38FF8CA647F001ED8DACC7F00164DA146DAFE8445B8C89999728AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CEB7D890E3377C531BA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B6945E48972B8D6B0376E601842F6C81A1F004C906525384307823802FF610243DF43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CD1D040B6C1ECEA3F43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5D4F383152B204702064A7996AAF06AA6713A8C9204E0BC5AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756665624D6DDF07B5410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C974B02B4EA30DFB906A49E1547BB71923F3439DA7082EDD03BFBA6ECB2B32856DE51A7FC7DC8B7E1D7E09C32AA3244CE12E5DC14D60346248B3438199C484D3725D5B54B2FE45753EB3F6AD6EA9203E X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXSF/Tsl6M2OTgvJDIPjtrw== X-Mailru-Sender: 4C235FE2E5D2D8909F78BBF75471DD8605886B22BF18AD175BBBEE1D5B66126F0901A01E0DC3C3E37B6BBE17E403543AD070B20000CAF116B128302DCECB6B19FE2C68CC745D971B112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] net.box: add __serialize and __tostring methods for future objects X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Yaroslav Dynnikov via Tarantool-patches Reply-To: Yaroslav Dynnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --00000000000063bad905ca7794da Content-Type: text/plain; charset="UTF-8" Hi! Thanks for the patch. While reviewing it, I've noticed that the autocompletion doesn't work for a net.box.future object. I guess it's a topic for another issue, isn't it? Besides that, LGTM. Best regards Yaroslav Dynnikov On Wed, 25 Aug 2021 at 16:35, Vladimir Davydov wrote: > On Wed, Aug 25, 2021 at 12:39:27AM +0200, Vladislav Shpilevoy wrote: > > > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > > > index 1783da607dcc..3ed464a93437 100644 > > > --- a/src/box/lua/net_box.c > > > +++ b/src/box/lua/net_box.c > > > @@ -1432,6 +1432,29 @@ luaT_netbox_request_gc(struct lua_State *L) > > > return 0; > > > } > > > > > > +static int > > > +luaT_netbox_request_tostring(struct lua_State *L) > > > +{ > > > + lua_pushstring(L, netbox_request_typename); > > > + return 1; > > > +} > > > + > > > +static int > > > +luaT_netbox_request_serialize(struct lua_State *L) > > > +{ > > > + struct netbox_request *request = luaT_check_netbox_request(L, 1); > > > + /* > > > + * If there are user-defined fields attached to the future object, > > > + * return them, otherwise push the type name, like __tostring does. > > > + */ > > > + if (request->index_ref != LUA_NOREF) { > > > + lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref); > > > + } else { > > > + lua_pushstring(L, netbox_request_typename); > > > + } > > > + return 1; > > > +} > > > > 1. It does not look good that __serialize might return both a table > > and a string depending on the object content. Perhaps it is worth to > > return an empty table when it has no members. Otherwise, say, if I > > store an optional value in the index and want to get it like this: > > > > serialized_req.member > > > > then I will get sometimes nil, sometimes an error would be thrown. > > > > Agree. Fixed the patch to return an empty table in case there's no user > data stored in the future object. > > > > + > > > static int > > > luaT_netbox_request_index(struct lua_State *L) > > > { > > > @@ -2107,6 +2130,8 @@ luaopen_net_box(struct lua_State *L) > > > > > > static const struct luaL_Reg netbox_request_meta[] = { > > > { "__gc", luaT_netbox_request_gc }, > > > + {"__tostring", luaT_netbox_request_tostring }, > > > + {"__serialize", luaT_netbox_request_serialize }, > > > > 2. You have a whitespace after { in all the other lines. Could you > > please add them here too? > > Sorry about that. Fixed. > > Incremental diff: > -- > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > index 3ed464a93437..7a72cb324df7 100644 > --- a/src/box/lua/net_box.c > +++ b/src/box/lua/net_box.c > @@ -1443,14 +1443,10 @@ static int > luaT_netbox_request_serialize(struct lua_State *L) > { > struct netbox_request *request = luaT_check_netbox_request(L, 1); > - /* > - * If there are user-defined fields attached to the future object, > - * return them, otherwise push the type name, like __tostring does. > - */ > if (request->index_ref != LUA_NOREF) { > lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref); > } else { > - lua_pushstring(L, netbox_request_typename); > + lua_newtable(L); > } > return 1; > } > @@ -2130,8 +2126,8 @@ luaopen_net_box(struct lua_State *L) > > static const struct luaL_Reg netbox_request_meta[] = { > { "__gc", luaT_netbox_request_gc }, > - {"__tostring", luaT_netbox_request_tostring }, > - {"__serialize", luaT_netbox_request_serialize }, > + { "__tostring", luaT_netbox_request_tostring }, > + { "__serialize", luaT_netbox_request_serialize }, > { "__index", luaT_netbox_request_index }, > { "__newindex", luaT_netbox_request_newindex }, > { "is_ready", luaT_netbox_request_is_ready }, > diff --git a/test/box/net.box_fiber-async_gh-3107.result > b/test/box/net.box_fiber-async_gh-3107.result > index 450934cd6593..98c9af953efa 100644 > --- a/test/box/net.box_fiber-async_gh-3107.result > +++ b/test/box/net.box_fiber-async_gh-3107.result > @@ -263,7 +263,7 @@ tostring(future) > ... > future > --- > -- net.box.request > +- [] > ... > future.abc = 123 > --- > --00000000000063bad905ca7794da Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi! Thanks for the patch.

While reviewing it, I'= ;ve noticed that the autocompletion doesn't work
for a net.box.futur= e object. I guess it's a topic for another issue,
isn't it? Besi= des that, LGTM.

Best regards
Yaroslav Dynnikov

On Wed, 25 Aug 2021= at 16:35, Vladimir Davydov <v= davydov@tarantool.org> wrote:
On Wed, Aug 25, 2021 at 12:39:27AM +0200, Vladislav Sh= pilevoy wrote:
> > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> > index 1783da607dcc..3ed464a93437 100644
> > --- a/src/box/lua/net_box.c
> > +++ b/src/box/lua/net_box.c
> > @@ -1432,6 +1432,29 @@ luaT_netbox_request_gc(struct lua_State *L= )
> >=C2=A0 =C2=A0 =C2=A0return 0;
> >=C2=A0 }
> >=C2=A0
> > +static int
> > +luaT_netbox_request_tostring(struct lua_State *L)
> > +{
> > +=C2=A0 =C2=A0lua_pushstring(L, netbox_request_typename);
> > +=C2=A0 =C2=A0return 1;
> > +}
> > +
> > +static int
> > +luaT_netbox_request_serialize(struct lua_State *L)
> > +{
> > +=C2=A0 =C2=A0struct netbox_request *request =3D luaT_check_netbo= x_request(L, 1);
> > +=C2=A0 =C2=A0/*
> > +=C2=A0 =C2=A0 * If there are user-defined fields attached to the= future object,
> > +=C2=A0 =C2=A0 * return them, otherwise push the type name, like = __tostring does.
> > +=C2=A0 =C2=A0 */
> > +=C2=A0 =C2=A0if (request->index_ref !=3D LUA_NOREF) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lua_rawgeti(L, LUA_REGI= STRYINDEX, request->index_ref);
> > +=C2=A0 =C2=A0} else {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lua_pushstring(L, netbo= x_request_typename);
> > +=C2=A0 =C2=A0}
> > +=C2=A0 =C2=A0return 1;
> > +}
>
> 1. It does not look good that __serialize might return both a table > and a string depending on the object content. Perhaps it is worth to > return an empty table when it has no members. Otherwise, say, if I
> store an optional value in the index and want to get it like this:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0serialized_req.member
>
> then I will get sometimes nil, sometimes an error would be thrown.
>

Agree. Fixed the patch to return an empty table in case there's no user=
data stored in the future object.

> > +
> >=C2=A0 static int
> >=C2=A0 luaT_netbox_request_index(struct lua_State *L)
> >=C2=A0 {
> > @@ -2107,6 +2130,8 @@ luaopen_net_box(struct lua_State *L)
> >=C2=A0
> >=C2=A0 =C2=A0 =C2=A0static const struct luaL_Reg netbox_request_me= ta[] =3D {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ "__gc"= ,=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0luaT_netbox_request_gc },
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{"__tostring"= ,=C2=A0 =C2=A0 =C2=A0 luaT_netbox_request_tostring },
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{"__serialize"= ;,=C2=A0 =C2=A0 =C2=A0luaT_netbox_request_serialize },
>
> 2. You have a whitespace after { in all the other lines. Could you
> please add them here too?

Sorry about that. Fixed.

Incremental diff:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 3ed464a93437..7a72cb324df7 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -1443,14 +1443,10 @@ static int
=C2=A0luaT_netbox_request_serialize(struct lua_State *L)
=C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct netbox_request *request =3D luaT_check_n= etbox_request(L, 1);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 * If there are user-defined fields attached to= the future object,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 * return them, otherwise push the type name, l= ike __tostring does.
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (request->index_ref !=3D LUA_NOREF) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lua_rawgeti(L, LUA_= REGISTRYINDEX, request->index_ref);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lua_pushstring(L, n= etbox_request_typename);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lua_newtable(L); =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
=C2=A0}
@@ -2130,8 +2126,8 @@ luaopen_net_box(struct lua_State *L)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 static const struct luaL_Reg netbox_request_met= a[] =3D {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 { "__gc",= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0luaT_netbox_request_gc },
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{"__tostring&q= uot;,=C2=A0 =C2=A0 =C2=A0 luaT_netbox_request_tostring },
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{"__serialize&= quot;,=C2=A0 =C2=A0 =C2=A0luaT_netbox_request_serialize },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ "__tostring&= quot;,=C2=A0 =C2=A0 =C2=A0luaT_netbox_request_tostring },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ "__serialize= ",=C2=A0 =C2=A0 luaT_netbox_request_serialize },
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 { "__index&quo= t;,=C2=A0 =C2=A0 =C2=A0 =C2=A0 luaT_netbox_request_index },
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 { "__newindex&= quot;,=C2=A0 =C2=A0 =C2=A0luaT_netbox_request_newindex },
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 { "is_ready&qu= ot;,=C2=A0 =C2=A0 =C2=A0 =C2=A0luaT_netbox_request_is_ready },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box= _fiber-async_gh-3107.result
index 450934cd6593..98c9af953efa 100644
--- a/test/box/net.box_fiber-async_gh-3107.result
+++ b/test/box/net.box_fiber-async_gh-3107.result
@@ -263,7 +263,7 @@ tostring(future)
=C2=A0...
=C2=A0future
=C2=A0---
-- net.box.request
+- []
=C2=A0...
=C2=A0future.abc =3D 123
=C2=A0---
--00000000000063bad905ca7794da--