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 6B68220EE5 for ; Wed, 28 Mar 2018 04:05:51 -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 yCHkyv6GxsEG for ; Wed, 28 Mar 2018 04:05:51 -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 2911A23BF7 for ; Wed, 28 Mar 2018 04:05:51 -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> <22af426b-439b-ef31-893d-1e677d1a478e@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Wed, 28 Mar 2018 11:05:49 +0300 MIME-Version: 1.0 In-Reply-To: <22af426b-439b-ef31-893d-1e677d1a478e@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" - bool names_only = 0; + bool names_only = false; 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. >> >>> 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