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

[Kirill Shcherbatov]

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