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

[v.shpilevoy at tarantool.org]

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.

> +	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 {}.

> +		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.

> +	}
> +
> 	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
> 
>