[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple

[Kirill Shcherbatov]

I've also updated commit message content.

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index f8583a2..2ff9a11 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -38,6 +38,7 @@ extern "C" {
  	#include <lua.h>
  	#include <lauxlib.h>
  	#include <lualib.h>
+	#include <trivia/util.h>
  } /* extern "C" */

  #include "box/space.h"
@@ -175,7 +176,7 @@ lbox_fillspace(struct lua_State *L, struct space 
*space, int i)
  	lua_pushstring(L, space->def->engine_name);
  	lua_settable(L, i);

-	/* space.schema_version */
+	/* space._schema_version */
  	lua_pushstring(L, "_schema_version");
  	luaL_pushuint64(L, box_schema_version());
  	lua_settable(L, i);
@@ -400,8 +401,8 @@ static struct trigger on_alter_space_in_lua = {
  /**
   * Get a C pointer for space structure.
   * Caches the result. Cleans the user(negative) stack itself.
- * @param Lua space object at top of the Lua stack
- * @return C structure pointer
+ * @param Lua space object at top of the Lua stack.
+ * @retval C structure pointer.
   */
  static int
  lbox_space_ptr_cached(struct lua_State *L)
@@ -422,6 +423,8 @@ lbox_space_ptr_cached(struct lua_State *L)
  		lua_getfield(L, 1, "id");
  		uint32_t id = luaL_touint64(L, -1);
  		space = space_by_id(id);
+		if (!space)
+			luaL_error(L, "Specified space is not exists");

  		/** update the cache */
  		luaL_pushuint64(L, global_shema_version);
@@ -435,11 +438,11 @@ lbox_space_ptr_cached(struct lua_State *L)
  }

  /**
- * Make a tuple or a table Lua object by map
- * @param Lua space object
- * @param Lua map table object
- * @param Lua opts table object (optional)
- * @return C structure pointer
+ * Make a tuple or a table Lua object by map.
+ * @param Lua space object.
+ * @param Lua map table object.
+ * @param Lua opts table object (optional).
+ * @retval C structure pointer.
   */
  static int
  lbox_space_frommap(struct lua_State *L)
@@ -461,7 +464,6 @@ lbox_space_frommap(struct lua_State *L)

  	lbox_space_ptr_cached(L);
  	space = (struct space *)lua_topointer(L, -1);
-
  	dict = space->format->dict;
  	lua_createtable(L, space->def->field_count, 0);

@@ -472,8 +474,9 @@ lbox_space_frommap(struct lua_State *L)
  		const char *key = lua_tolstring(L, -2, &key_len);
  		uint32_t key_hash = lua_hashstring(L, -2);
  		if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno)) {
+			const char *err_msg = tt_sprintf("Unknown field '%s'", key);
  			lua_pushnil(L);
-			lua_pushstring(L, "Unknown field");
+			lua_pushstring(L, err_msg);
  			return 2;
  		}
  		lua_rawseti(L, -3, fieldno+1);


On 02.04.2018 14:19, v.shpilevoy at tarantool.org wrote:
> Hello. Thanks for your contributing! A several (5) minor fixes are left:
> 
> 1. Please, describe the patch in a commit message body. It would be good, if you
> write why the feature is needed, who needs this, and very short description of
> the feature itself: API usage example, options description. See Vladimir Davydov
> patches as the perfect example.
> 
>> 	/* space.schema_version */
>> 	lua_pushstring(L, "_schema_version");
>> 	luaL_pushuint64(L, box_schema_version());
>> 	lua_settable(L, i);
> 
> 2. Please, update the comment too: /* space._schema_version */ (added '_').
> 
>> /**
>>   * Get a C pointer for space structure.
>>   * Caches the result. Cleans the user(negative) stack itself.
>>   * @param Lua space object at top of the Lua stack
>>   * @return C structure pointer
>>   */
>> static int
>> lbox_space_ptr_cached(struct lua_State *L)
> 
> 3. Please, use @retval instead of @return, and put '.' at the end of a sentence.
> 
> 4. Seems like the following test crashes Tarantool. Please, fix it.
> 
> box.cfg{}
> format = {{'field1', 'unsigned'}}
> s = box.schema.create_space('test', {format = format})
> pk = s:create_index('pk')
> s:drop()
> s:frommap({field1 = 100})
> 
> Process 67761 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xe8)
>      frame #0: 0x0000000100100b68 tarantool`lbox_space_frommap(L=0x0000000103200378) at space.cc:465
>     462 		lbox_space_ptr_cached(L);
>     463 		space = (struct space *)lua_topointer(L, -1);
>     464 	
> -> 465 		dict = space->format->dict;
>     466 		lua_createtable(L, space->def->field_count, 0);
>     467 	
>     468 		lua_pushnil(L);
> Target 0: (tarantool) stopped.
> 
> 
> 
>> 		if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno)) {
>> 			lua_pushnil(L);
>> 			lua_pushstring(L, "Unknown field");
>> 			return 2;
>> 		}
>> 		lua_rawseti(L, -3, fieldno+1);
> 
> 
> 5. Please, return field name into error message. In the previous patch the error message
> was ok. Just do not throw it. And put spaces before and after arithmetic operations.
> 
>