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 EB2F720EE5 for ; Wed, 28 Mar 2018 04:01:12 -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 4DlAsXQsPjZY for ; Wed, 28 Mar 2018 04:01:12 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 AA23F20E10 for ; Wed, 28 Mar 2018 04:01:12 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option References: <1397a5af1ab577c4a04c14a2ccb779b34fa21de3.1522162837.git.kshcherbatov@tarantool.org> <33B37A8B-180B-44D6-83E9-A10F028D03A3@tarantool.org> From: Kirill Shcherbatov Message-ID: <22af426b-439b-ef31-893d-1e677d1a478e@tarantool.org> Date: Wed, 28 Mar 2018 11:01:10 +0300 MIME-Version: 1.0 In-Reply-To: <33B37A8B-180B-44D6-83E9-A10F028D03A3@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "v.shpilevoy@tarantool.org" 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. > >> 27 марта 2018 г., в 18:00, Kirill Shcherbatov написал(а): >> >> 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(-) >> >> 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 = lua_gettop(L); >> + if (argc < 1 || argc > 2) >> + goto error; >> + int names_only = 0; > > 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 = lua_gettop(L); if (argc < 1 || argc > 2) goto error; - int names_only = 0; + bool names_only = 0; if (argc == 2) { > >> + if (argc == 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 {}. 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; - } > >> + names_only = 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. > names_only = lua_toboolean(L, -1); - lua_pop(L, 1); } const struct tuple *tuple = 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 = lua_checktuple(L, 1); >> const struct tuple_format *format = tuple_format(tuple); >> const struct tuple_field *field = &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 = 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; >> } >> >> /** >> 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 = t1:tomap() >> maplen(t1map), t1map[1], t1map[2], t1map[3] >> s:drop() >> >> +-- >> +-- gh-2821: tuple:tomap() names_only feature. >> +-- >> +format = {} >> +format[1] = {name = 'field1', type = 'unsigned' } >> +format[2] = {name = 'field2', type = 'unsigned' } >> +s = box.schema.create_space('test', {format = format}) >> +pk = s:create_index('pk') >> +t = s:replace{100, 200, 300 } >> +t:tomap({names_only = false}) >> +t:tomap({names_only = true}) >> +t:tomap({names_only = 'text'}) >> +t:tomap({names_only = true}, {dummy = true}) >> +t:tomap({}) >> +s:drop() > > 5. What if I create a space with no format, and will call > tomap({names_only = true}) on its tuple? > >> + >> engine = nil >> test_run = nil >> -- >> 2.7.4 >> >> > > 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 = true}) ... t:tomap({names_only = 'text'}) --- -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})' +- error: 'Usage: tuple:tomap(opts)' ... t:tomap({names_only = true}, {dummy = true}) --- -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})' +- error: 'Usage: tuple:tomap(opts)' ... t:tomap({}) --- @@ -643,6 +643,22 @@ t:tomap({}) s:drop() --- ... +s = box.schema.create_space('test') +--- +... +pk = s:create_index('pk') +--- +... +t = s:replace{1,2,3,4,5,6,7} +--- +... +t:tomap({names_only = true}) +--- +- [] +... +s:drop() +--- +... engine = 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 = 'text'}) t:tomap({names_only = true}, {dummy = true}) t:tomap({}) s:drop() +s = box.schema.create_space('test') +pk = s:create_index('pk') +t = s:replace{1,2,3,4,5,6,7} +t:tomap({names_only = true}) +s:drop() engine = nil test_run = nil --