[Tarantool-patches] [PATCH] box: frommap() bug fix
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Nov 27 00:01:24 MSK 2019
Thanks for the patch!
See 4 comments below.
On 24/11/2019 22:18, Chris Sosnin wrote:
> - If an optional argument is provided for
> space_object:frommap() (which is {table = true|false}),
> type match for first arguments is omitted, which is
> incorrect. We should return the result only after making
> sure it is possible to build a tuple.
1. This is a good catch. Please, provide a test. To ensure,
that it won't break again in the future.
I think you fit it in an existing test file, where other
frommap() cases are checked. It is not worth creating a new
test file just for this tiny ticket.
>
> - If there is a type mismatch, however, frommap() does not
> return nil, err as it is mentioned in the description, so we
> change it to be this way.
>
> Closes: #4262
> ---
2. All the same as for #4515 email.
> src/box/lua/space.cc | 11 +++++++----
> test/box/tuple.result | 3 ++-
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index f6e96f0c0..a4ffa8240 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -580,14 +580,17 @@ lbox_space_frommap(struct lua_State *L)
> }
> lua_rawseti(L, -3, fieldno+1);
> }
> - if (table)
> - return 1;
>
> lua_replace(L, 1);
> lua_settop(L, 1);
> tuple = luaT_tuple_new(L, -1, space->format);
> - if (tuple == NULL)
> - return luaT_error(L);
> + if (tuple == NULL) {
> + struct error *e = diag_last_error(&fiber()->diag);
> + lua_pushnil(L);
> + lua_pushstring(L, e->errmsg);
> + return 2;
> + } else if (table)
> + return 1;
3. Nit: I think you may omit 'else' keyword here. Anyway
the previous condition makes a return.
4. When any if's or else's body has {}, all the others
in the same chain should have it as well. So 'return 1;'
should also be in {}.
> luaT_pushtuple(L, tuple);
> return 1;
> usage_error:
More information about the Tarantool-patches
mailing list