[PATCH v2 3/6] lua: add luaT_newtuple()
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Jan 10 15:44:46 MSK 2019
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);
More information about the Tarantool-patches
mailing list