[tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option

[Kirill Shcherbatov <kshcherbatov@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 
>>> <kshcherbatov@tarantool.org> написал(а):
>>>
>>> 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