Re: [PATCH v2 3/6] lua: add luaT_newtuple()

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Wed, Jan 09, 2019 at 11:20:11PM +0300, Alexander Turenko wrote:
> The function allows to create a tuple with specific tuple format in C
> code using a Lua table, an another tuple or objects on a Lua stack.
> 
> Needed for #3276.
> ---
>  src/box/lua/tuple.c | 91 +++++++++++++++++++++++++++++++++------------
>  src/box/lua/tuple.h | 15 ++++++++
>  2 files changed, 83 insertions(+), 23 deletions(-)

Although a test would be nice to have, I guess we can live without it,
because the new function is tested indirectly via lbox_tuple_new().

> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 1867f810f..7e9ad89fe 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -92,6 +92,65 @@ luaT_istuple(struct lua_State *L, int narg)
>  	return *(struct tuple **) data;
>  }
>  
> +struct tuple *
> +luaT_newtuple(struct lua_State *L, int idx, box_tuple_format_t *format)

I looked at the Lua reference manual and realized that they usually call
a function lua_newsomething if it creates an object on Lua stack. So I
guess we'd better rename it to luaT_tuple_new() to avoid confusion.

> +{
> +	struct tuple *tuple;
> +
> +	if (idx == 0 || lua_istable(L, idx)) {
> +		struct ibuf *buf = tarantool_lua_ibuf;
> +		ibuf_reset(buf);
> +		struct mpstream stream;
> +		mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> +		      luamp_error, L);

Nit: bad indentation.

> +		if (idx == 0) {
> +			/*
> +			 * Create the tuple from lua stack
> +			 * objects.
> +			 */
> +			int argc = lua_gettop(L);
> +			mpstream_encode_array(&stream, argc);
> +			for (int k = 1; k <= argc; ++k) {
> +				luamp_encode(L, luaL_msgpack_default, &stream,
> +					     k);
> +			}
> +		} else {
> +			/* Create the tuple from a Lua table. */
> +			luamp_encode_tuple(L, luaL_msgpack_default, &stream,
> +					   idx);
> +		}
> +		mpstream_flush(&stream);
> +		tuple = box_tuple_new(format, buf->buf,
> +				      buf->buf + ibuf_used(buf));
> +		if (tuple == NULL) {
> +			luaT_pusherror(L, diag_last_error(diag_get()));

Why not simply throw the error with luaT_error()? Other similar
functions throw an error, not just push it to the stack.

> +			return NULL;
> +		}
> +		ibuf_reinit(tarantool_lua_ibuf);
> +		return tuple;
> +	}
> +
> +	tuple = luaT_istuple(L, idx);
> +	if (tuple == NULL) {
> +		lua_pushfstring(L, "A tuple or a table expected, got %s",
> +				lua_typename(L, lua_type(L, -1)));
> +		return NULL;
> +	}
> +
> +	/*
> +	 * Create the new tuple with the necessary format from

Nit: a new tuple

> +	 * the another tuple.

Nit: 'the' is redundant.

> +	 */
> +	const char *tuple_beg = tuple_data(tuple);
> +	const char *tuple_end = tuple_beg + tuple->bsize;
> +	tuple = box_tuple_new(format, tuple_beg, tuple_end);
> +	if (tuple == NULL) {
> +		luaT_pusherror(L, diag_last_error(diag_get()));
> +		return NULL;
> +	}
> +	return tuple;

I see that you reworked the original code so as to avoid tuple data
copying in case a new tuple is created from another tuple. That's OK,
but I think that it should've been done in a separate patch.

> +}
> +
>  int
>  lbox_tuple_new(lua_State *L)
>  {
> @@ -100,33 +159,19 @@ lbox_tuple_new(lua_State *L)
>  		lua_newtable(L); /* create an empty tuple */
>  		++argc;
>  	}
> -	struct ibuf *buf = tarantool_lua_ibuf;
> -
> -	ibuf_reset(buf);
> -	struct mpstream stream;
> -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> -		      luamp_error, L);
> -
> -	if (argc == 1 && (lua_istable(L, 1) || luaT_istuple(L, 1))) {
> -		/* New format: box.tuple.new({1, 2, 3}) */
> -		luamp_encode_tuple(L, luaL_msgpack_default, &stream, 1);
> -	} else {
> -		/* Backward-compatible format: box.tuple.new(1, 2, 3). */
> -		mpstream_encode_array(&stream, argc);
> -		for (int k = 1; k <= argc; ++k) {
> -			luamp_encode(L, luaL_msgpack_default, &stream, k);
> -		}
> -	}
> -	mpstream_flush(&stream);
> -
> +	/*
> +	 * Use backward-compatible parameters format:
> +	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
> +	 * box.tuple.new({1, 2, 3}) (idx == 1).
> +	 */
> +	int idx = argc == 1 && (lua_istable(L, 1) ||
> +		luaT_istuple(L, 1));
>  	box_tuple_format_t *fmt = box_tuple_format_default();
> -	struct tuple *tuple = box_tuple_new(fmt, buf->buf,
> -					   buf->buf + ibuf_used(buf));
> +	struct tuple *tuple = luaT_newtuple(L, idx, fmt);
>  	if (tuple == NULL)
> -		return luaT_error(L);
> +		return lua_error(L);
>  	/* box_tuple_new() doesn't leak on exception, see public API doc */
>  	luaT_pushtuple(L, tuple);
> -	ibuf_reinit(tarantool_lua_ibuf);
>  	return 1;
>  }
>  
> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 5d7062eb8..3319b951e 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -41,6 +41,8 @@ typedef struct tuple box_tuple_t;
>  struct lua_State;
>  struct mpstream;
>  struct luaL_serializer;
> +struct tuple_format;
> +typedef struct tuple_format box_tuple_format_t;
>  
>  /** \cond public */
>  
> @@ -66,6 +68,19 @@ luaT_istuple(struct lua_State *L, int idx);
>  
>  /** \endcond public */
>  
> +/**
> + * Create the new tuple with specific format from a Lua table, a

Nit: a new tuple

> + * tuple or objects on the lua stack.

Nit: comma before 'or' is missing ;-)

> + *
> + * Set idx to zero to create the new tuple from objects on the lua
> + * stack.
> + *
> + * In case of an error push the error message to the Lua stack and
> + * return NULL.
> + */
> +struct tuple *
> +luaT_newtuple(struct lua_State *L, int idx, box_tuple_format_t *format);
> +
>  int
>  lbox_tuple_new(struct lua_State *L);