[tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Wed Mar 28 11:16:52 MSK 2018
LGTM. Vova, please, take a look.
> 28 марта 2018 г., в 11:05, Kirill Shcherbatov <kshcherbatov at 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 at tarantool.org wrote:
>>> See below 5 comments.
>>>
>>>> 27 марта 2018 г., в 18:00, Kirill Shcherbatov <kshcherbatov at 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
>
More information about the Tarantool-patches
mailing list