From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 68B792A770 for ; Mon, 2 Apr 2018 15:42:35 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oobbLOqB9WEn for ; Mon, 2 Apr 2018 15:42:35 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 279B12A767 for ; Mon, 2 Apr 2018 15:42:34 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple References: <90ece9a705197ded006ef50155f3b1bfca4c8d8f.1522261662.git.kshcherbatov@tarantool.org> <7a5c0afb-c030-fcd2-7d86-5c92cd7fa850@tarantool.org> From: Kirill Shcherbatov Message-ID: <01c64ee0-2344-41ac-1ff3-b39172d99fd4@tarantool.org> Date: Mon, 2 Apr 2018 22:42:32 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" 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 #include #include + #include } /* 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@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. > >