From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: Re: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option From: "v.shpilevoy@tarantool.org" In-Reply-To: Date: Wed, 28 Mar 2018 11:16:52 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <50FD81D5-916C-4E8C-B32B-1234729D0B40@tarantool.org> References: <1397a5af1ab577c4a04c14a2ccb779b34fa21de3.1522162837.git.kshcherbatov@tarantool.org> <33B37A8B-180B-44D6-83E9-A10F028D03A3@tarantool.org> <22af426b-439b-ef31-893d-1e677d1a478e@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladimir Davydov List-ID: LGTM. Vova, please, take a look. > 28 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 11:05, Kirill = Shcherbatov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0): >=20 > - bool names_only =3D 0; > + bool names_only =3D false; >=20 > On 28.03.2018 11:01, Kirill Shcherbatov wrote: >> Accounted your comments; A new test with empty tuple. Rebased on 1.9 >> On 27.03.2018 18:34, v.shpilevoy@tarantool.org wrote: >>> See below 5 comments. >>>=20 >>>> 27 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 18:00, = Kirill Shcherbatov =D0=BD=D0=B0=D0=BF=D0=B8=D1= =81=D0=B0=D0=BB(=D0=B0): >>>>=20 >>>> Fixes #3280 >>>> --- >>>> src/box/lua/tuple.c | 24 +++++++++++++++++++-- >>>> test/engine/tuple.result | 53 = ++++++++++++++++++++++++++++++++++++++++++++++ >>>> test/engine/tuple.test.lua | 16 ++++++++++++++ >>>> 3 files changed, 91 insertions(+), 2 deletions(-) >>>>=20 >>>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c >>>> index d5ed7c4..b52072c 100644 >>>> --- a/src/box/lua/tuple.c >>>> +++ b/src/box/lua/tuple.c >>>> @@ -280,8 +280,21 @@ luamp_encode_extension_box(struct lua_State = *L, int idx, >>>> static int >>>> lbox_tuple_to_map(struct lua_State *L) >>>> { >>>> - if (lua_gettop(L) < 1) >>>> - luaL_error(L, "Usage: tuple:tomap()"); >>>> + int argc =3D lua_gettop(L); >>>> + if (argc < 1 || argc > 2) >>>> + goto error; >>>> + int names_only =3D 0; >>>=20 >>> 1. As I noted in the previous reviews, use 'bool' for flags. Not = integer. >> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c >> index b52072c..00be718 100644 >> --- a/src/box/lua/tuple.c >> +++ b/src/box/lua/tuple.c >> @@ -283,16 +283,14 @@ lbox_tuple_to_map(struct lua_State *L) >> int argc =3D lua_gettop(L); >> if (argc < 1 || argc > 2) >> goto error; >> - int names_only =3D 0; >> + bool names_only =3D 0; >> if (argc =3D=3D 2) { >>>=20 >>>> + if (argc =3D=3D 2) { >>>> + if (!lua_istable(L, 2)) >>>> + goto error; >>>> + lua_getfield(L, 2, "names_only"); >>>> + if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) { >>>> + goto error; >>>> + } >>>=20 >>> 2. We put 'if' body in {} only if it consists of multiple lines. For = single line >>> do not use {}. >> if (!lua_istable(L, 2)) >> goto error; >> lua_getfield(L, 2, "names_only"); >> - if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) { >> + if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) >> goto error; >> - } >>>=20 >>>> + names_only =3D lua_toboolean(L, -1); >>>> + lua_pop(L, 1); >>>=20 >>> 3. As I already wrote, you should not pop temporary values from the = stack - >>> Lua does it for you. >>>=20 >> names_only =3D lua_toboolean(L, -1); >> - lua_pop(L, 1); >> } >> const struct tuple *tuple =3D lua_checktuple(L, 1); >> @@ -327,7 +325,7 @@ lbox_tuple_to_map(struct lua_State *L) >> } >> return 1; >> error: >> - luaL_error(L, "Usage: tuple:tomap(params)"); >> + luaL_error(L, "Usage: tuple:tomap(opts)"); >> return 1; >> } >>>> + } >>>> + >>>> const struct tuple *tuple =3D lua_checktuple(L, 1); >>>> const struct tuple_format *format =3D tuple_format(tuple); >>>> const struct tuple_field *field =3D &format->fields[0]; >>>> @@ -294,6 +307,8 @@ lbox_tuple_to_map(struct lua_State *L) >>>> lua_pushstring(L, field->name); >>>> luamp_decode(L, luaL_msgpack_default, &pos); >>>> lua_rawset(L, -3); >>>> + if (names_only) >>>> + continue; >>>> /* >>>> * Access the same field by an index. There is no >>>> * copy for tables - lua optimizes it and uses >>>> @@ -303,12 +318,17 @@ lbox_tuple_to_map(struct lua_State *L) >>>> lua_rawget(L, -2); >>>> lua_rawseti(L, -2, i + TUPLE_INDEX_BASE); >>>> } >>>> + if (names_only) >>>> + return 1; >>>> /* Access for not named fields by index. */ >>>> for (int i =3D n_named; i < field_count; ++i) { >>>> luamp_decode(L, luaL_msgpack_default, &pos); >>>> lua_rawseti(L, -2, i + TUPLE_INDEX_BASE); >>>> } >>>> return 1; >>>> +error: >>>> + luaL_error(L, "Usage: tuple:tomap(params)"); >>>=20 >>> 4. I asked you to write 'opts', not 'params'. Tomap accept not = multiple >>> parameters, but single opts map. >>>=20 >>>> + return 1; >>>> } >>>>=20 >>>> /** >>>> diff --git a/test/engine/tuple.test.lua = b/test/engine/tuple.test.lua >>>> index 6d7d254..3411947 100644 >>>> --- a/test/engine/tuple.test.lua >>>> +++ b/test/engine/tuple.test.lua >>>> @@ -200,5 +200,21 @@ t1map =3D t1:tomap() >>>> maplen(t1map), t1map[1], t1map[2], t1map[3] >>>> s:drop() >>>>=20 >>>> +-- >>>> +-- gh-2821: tuple:tomap() names_only feature. >>>> +-- >>>> +format =3D {} >>>> +format[1] =3D {name =3D 'field1', type =3D 'unsigned' } >>>> +format[2] =3D {name =3D 'field2', type =3D 'unsigned' } >>>> +s =3D box.schema.create_space('test', {format =3D format}) >>>> +pk =3D s:create_index('pk') >>>> +t =3D s:replace{100, 200, 300 } >>>> +t:tomap({names_only =3D false}) >>>> +t:tomap({names_only =3D true}) >>>> +t:tomap({names_only =3D 'text'}) >>>> +t:tomap({names_only =3D true}, {dummy =3D true}) >>>> +t:tomap({}) >>>> +s:drop() >>>=20 >>> 5. What if I create a space with no format, and will call >>> tomap({names_only =3D true}) on its tuple? >>>=20 >>>> + >>>> engine =3D nil >>>> test_run =3D nil >>>> --=20 >>>> 2.7.4 >>>>=20 >>>>=20 >>>=20 >>>=20 >> diff --git a/test/engine/tuple.result b/test/engine/tuple.result >> index f9805b0..9fd9bdc 100644 >> --- a/test/engine/tuple.result >> +++ b/test/engine/tuple.result >> @@ -626,11 +626,11 @@ t:tomap({names_only =3D true}) >> ... >> t:tomap({names_only =3D 'text'}) >> --- >> -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only =3D = true})' >> +- error: 'Usage: tuple:tomap(opts)' >> ... >> t:tomap({names_only =3D true}, {dummy =3D true}) >> --- >> -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only =3D = true})' >> +- error: 'Usage: tuple:tomap(opts)' >> ... >> t:tomap({}) >> --- >> @@ -643,6 +643,22 @@ t:tomap({}) >> s:drop() >> --- >> ... >> +s =3D box.schema.create_space('test') >> +--- >> +... >> +pk =3D s:create_index('pk') >> +--- >> +... >> +t =3D s:replace{1,2,3,4,5,6,7} >> +--- >> +... >> +t:tomap({names_only =3D true}) >> +--- >> +- [] >> +... >> +s:drop() >> +--- >> +... >> engine =3D nil >> --- >> ... >> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua >> index 3411947..0af93ca 100644 >> --- a/test/engine/tuple.test.lua >> +++ b/test/engine/tuple.test.lua >> @@ -215,6 +215,11 @@ t:tomap({names_only =3D 'text'}) >> t:tomap({names_only =3D true}, {dummy =3D true}) >> t:tomap({}) >> s:drop() >> +s =3D box.schema.create_space('test') >> +pk =3D s:create_index('pk') >> +t =3D s:replace{1,2,3,4,5,6,7} >> +t:tomap({names_only =3D true}) >> +s:drop() >> engine =3D nil >> test_run =3D nil >=20