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 74371274CC for ; Mon, 2 Apr 2018 07:19:38 -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 9jgtCHfIiMjo for ; Mon, 2 Apr 2018 07:19:38 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 331A8270AE for ; Mon, 2 Apr 2018 07:19:37 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple From: "v.shpilevoy@tarantool.org" In-Reply-To: <7a5c0afb-c030-fcd2-7d86-5c92cd7fa850@tarantool.org> Date: Mon, 2 Apr 2018 14:19:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <90ece9a705197ded006ef50155f3b1bfca4c8d8f.1522261662.git.kshcherbatov@tarantool.org> <7a5c0afb-c030-fcd2-7d86-5c92cd7fa850@tarantool.org> 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: Kirill Shcherbatov 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 =3D {{'field1', 'unsigned'}} s =3D box.schema.create_space('test', {format =3D format}) pk =3D s:create_index('pk') s:drop() s:frommap({field1 =3D 100}) Process 67761 stopped * thread #1, queue =3D 'com.apple.main-thread', stop reason =3D = EXC_BAD_ACCESS (code=3D1, address=3D0xe8) frame #0: 0x0000000100100b68 = tarantool`lbox_space_frommap(L=3D0x0000000103200378) at space.cc:465 462 lbox_space_ptr_cached(L); 463 space =3D (struct space *)lua_topointer(L, -1); 464 =09 -> 465 dict =3D space->format->dict; 466 lua_createtable(L, space->def->field_count, 0); 467 =09 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.