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 DE9F6293D2 for ; Thu, 29 Mar 2018 06:14:24 -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 o_cU8-Rcna5V for ; Thu, 29 Mar 2018 06:14:24 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 77786207A6 for ; Thu, 29 Mar 2018 06:14:24 -0400 (EDT) From: "v.shpilevoy@tarantool.org" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_FB2F7B4C-993D-44B4-80F3-29488FCECE30" 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 Date: Thu, 29 Mar 2018 13:14:21 +0300 In-Reply-To: References: <90ece9a705197ded006ef50155f3b1bfca4c8d8f.1522261662.git.kshcherbatov@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: Kirill Shcherbatov Cc: tarantool-patches@freelists.org --Apple-Mail=_FB2F7B4C-993D-44B4-80F3-29488FCECE30 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii See below 10 comments. > diff --git a/src/box/lua/space.cc = b/src/box/lua/space.cc > index 29a9aca..a8cfb14 100644 > --- a/src/box/lua/space.cc > +++ b/src/box/lua/space.cc > @@ -32,6 +32,7 @@ > #include "box/lua/tuple.h" > #include "lua/utils.h" > #include "lua/trigger.h" > +#include "box/schema.h" >=20 > extern "C" { > #include > @@ -174,6 +175,16 @@ lbox_fillspace(struct lua_State *L, struct space = *space, int i) > lua_pushstring(L, space->def->engine_name); > lua_settable(L, i); >=20 > + /* space.schema_version */ > + lua_pushstring(L, "schema_version"); > + luaL_pushuint64(L, box_schema_version()); > + lua_settable(L, i); 1. Lets name it '_schema_version' - it is internal field. > + > + /* space._space */ > + lua_pushstring(L, "_space"); > + lua_pushlightuserdata(L, space); > + lua_settable(L, i); 2. Please, find a better name for the pointer. For example, 'ptr' - then = it will be accessed as space.ptr, that looks slightly better than = space._space. > +static int > +lbox_space_ptr_cached(struct lua_State *L) > +{ 3. Add a comment, what this function does, why, and what arguments on a = stack it expects, what and home many values returns. > + // take care about stack to call this function in frommap = directly > + lua_getfield(L, 1, "schema_version"); 4. A comment seems to be not linked with the code below. And please, do = not use '//' comments. In our code we use '/* ... */' for comments inside a function. Note, that comment max length is 66 symbols, including = indentation before a comment. > + > + void *space =3D nullptr; > + if (schema_version =3D=3D global_shema_version) { > + 5. Do not use nullptr. Everywhere in Tarantool code only NULL is used. > + bool tuple =3D false; 6. Lets return tuple by default, and name the option 'table' instead of tuple. If a user wants a table, then he must pass the option {table =3D = true}. > + if (argc =3D=3D 3) { > + if (!lua_istable(L, 3)) > + goto error; > + lua_getfield(L, 3, "tuple"); > + if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) > + goto error; > + tuple =3D lua_toboolean(L, -1); > + } > + 7. Double tabs prior to 'goto error'. > + if (tuple_fieldno_by_name(dict, key, key_len, key_hash, = &fieldno)) > + luaL_error(L, "Unknown field '%s'", key); 8. According to our Lua error returning convention, you can raise an = error either on incorrect usage (bad argument type, for example), or on OOM(Out Of = Memory) errors. On other errors you must return two values: nil and error object = or description. In this example you must return nil and "Unknown field = '%s'" string. > + lua_replace(L, 1); > + lua_settop(L, 1); > + return lbox_tuple_new(L); 9. How about a comment what is happening here? 10. Add a test on nil and box.NULL fields. And test, that a result of frommap() actually can be inserted or replaced into a space. --Apple-Mail=_FB2F7B4C-993D-44B4-80F3-29488FCECE30 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
See below 10 comments.

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 29a9aca..a8cfb14 = 100644
--- = a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -32,6 +32,7 = @@
#include = "box/lua/tuple.h"
#include "lua/utils.h"
#include "lua/trigger.h"
+#include "box/schema.h"

extern = "C" {
#include <lua.h>
@@ -174,6 +175,16 @@ = 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 */
+ lua_pushstring(L, = "schema_version");
+ luaL_pushuint64(L, = box_schema_version());
+ lua_settable(L, i);

1. Lets name it = '_schema_version' - it is internal field.

+
+ /* space._space */
+ lua_pushstring(L, "_space");
+ lua_pushlightuserdata(L, space);
+ lua_settable(L, i);

2. Please, find a better = name for the pointer. For example, 'ptr' - then it
will be accessed as space.ptr, that looks slightly better = than space._space.

+static int
+lbox_space_ptr_cached(struct lua_State *L)
+{

3. = Add a comment, what this function does, why, and what arguments on a = stack
it expects, what and home many values = returns.

+ // take care about stack to call this = function in frommap directly
+ lua_getfield(L, 1, = "schema_version");

4. A comment seems to be not linked with the code below. And = please, do not
use '//' comments. In our code we = use '/* ... */' for comments inside a
function. = Note, that comment max length is 66 symbols, including = indentation
before a comment.

+
+ void *space =3D nullptr;
+ if (schema_version =3D=3D = global_shema_version) {
+

5. = Do not use nullptr. Everywhere in Tarantool code only NULL is = used.

+ bool tuple =3D false;

6. Lets return tuple by default, and = name the option 'table' instead of
tuple. If a user = wants a table, then he must pass the option {table =3D true}.

+ if (argc =3D=3D 3) {
+ = if = (!lua_istable(L, 3))
+ = goto error;
+ lua_getfield(L, 3, "tuple");
+ = if = (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
+ = goto error;
+ tuple =3D lua_toboolean(L, -1);
+ }
+

7. = Double tabs prior to 'goto error'.

+ if (tuple_fieldno_by_name(dict, key, = key_len, key_hash, &fieldno))
+ = luaL_error(L, "Unknown field '%s'", = key);

8. According to our Lua error returning convention, you can = raise an error either
on incorrect usage (bad = argument type, for example), or on OOM(Out Of Memory)
errors. On other errors you must return two values: nil and = error object or
description. In this example you = must return nil and "Unknown field '%s'" string.

+ lua_replace(L, 1);
+ lua_settop(L, 1);
+ return = lbox_tuple_new(L);

9. How about a comment what is happening here?

10. Add a test on nil = and box.NULL fields. And test, that a result of
frommap() actually can be inserted or replaced into a = space.


= --Apple-Mail=_FB2F7B4C-993D-44B4-80F3-29488FCECE30--