From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2B4F929763 for ; Tue, 27 Mar 2018 11:34:40 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1iIYWCeivqGb for ; Tue, 27 Mar 2018 11:34:40 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CDDF72A2E4 for ; Tue, 27 Mar 2018 11:34:39 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option From: "v.shpilevoy@tarantool.org" In-Reply-To: <1397a5af1ab577c4a04c14a2ccb779b34fa21de3.1522162837.git.kshcherbatov@tarantool.org> Date: Tue, 27 Mar 2018 18:34:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <33B37A8B-180B-44D6-83E9-A10F028D03A3@tarantool.org> References: <1397a5af1ab577c4a04c14a2ccb779b34fa21de3.1522162837.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov See below 5 comments. > 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; 1. As I noted in the previous reviews, use 'bool' for flags. Not = integer. > + 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; > + } 2. We put 'if' body in {} only if it consists of multiple lines. For = single line do not use {}. > + names_only =3D lua_toboolean(L, -1); > + lua_pop(L, 1); 3. As I already wrote, you should not pop temporary values from the = stack - Lua does it for you. > + } > + > 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)"); 4. I asked you to write 'opts', not 'params'. Tomap accept not multiple parameters, but single opts map. > + 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() 5. What if I create a space with no format, and will call tomap({names_only =3D true}) on its tuple? > + > engine =3D nil > test_run =3D nil > --=20 > 2.7.4 >=20 >=20