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

["v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

See below 10 comments.

> diff --git a/src/box/lua/space.cc <http://space.cc/> b/src/box/lua/space.cc <http://space.cc/>
> index 29a9aca..a8cfb14 100644
> --- a/src/box/lua/space.cc <http://space.cc/>
> +++ b/src/box/lua/space.cc <http://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 = nullptr;
> +	if (schema_version == global_shema_version) {
> +

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

> +	bool tuple = 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 = true}.

> +	if (argc == 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 = 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.



[-- Attachment #2: Type: text/html, Size: 15052 bytes --]