[PATCH v3 5/6] box: refactor box_lua_find helper
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jun 18 17:22:04 MSK 2019
On Thu, Jun 13, 2019 at 05:08:25PM +0300, Kirill Shcherbatov wrote:
> The box_lua_find routine used to work with an empty stack only.
> It is unacceptable in following patches because this helper
> need to be reused in following patches for environment table
> construction.
>
> Refactored function is renamed to luaT_func_find.
>
> Needed for #4182, #1260
> ---
> src/box/lua/call.c | 54 +++++++++++++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 23eb29eb8..90c28a160 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -54,21 +54,19 @@
> * on top of the stack.
> */
> static int
> -box_lua_find(lua_State *L, const char *name, const char *name_end)
> +luaT_func_find(lua_State *L, const char *name, const char *name_end,
> + int *count)
Don't see any point in renaming it or using luaT_ prefix for this and
other helper functions in this compilation unit - other functions in the
same file have box_ prefix, e.g. box_process_lua. I may be wrong, but it
looks like luaT_ prefix is primarily used for generic helper functions
from util.c.
> {
> int index = LUA_GLOBALSINDEX;
> - int objstack = 0;
> + int objstack = 0, top = lua_gettop(L);
> const char *start = name, *end;
>
> while ((end = (const char *) memchr(start, '.', name_end - start))) {
> lua_checkstack(L, 3);
> lua_pushlstring(L, start, end - start);
> lua_gettable(L, index);
> - if (! lua_istable(L, -1)) {
> - diag_set(ClientError, ER_NO_SUCH_PROC,
> - name_end - name, name);
Why drop diag_set? Looking at the following patch, I see that you want
to set another error when you create a sandbox. Well, it's okay to
overwrite a diag. Moreover, we have plans to implement stacked diag so
that instead of overwriting the error, subsequent diag_set would append
a new error to the diag and we could print something like a stack trace
leading to the error.
> - luaT_error(L);
> - }
> + if (! lua_istable(L, -1))
> + return -1;
> start = end + 1; /* next piece of a.b.c */
> index = lua_gettop(L); /* top of the stack */
> }
> @@ -95,25 +90,27 @@ box_lua_find(lua_State *L, const char *name, const char *name_end)
> if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) {
> /* lua_call or lua_gettable would raise a type error
> * for us, but our own message is more verbose. */
> - diag_set(ClientError, ER_NO_SUCH_PROC,
> - name_end - name, name);
> - luaT_error(L);
> + return -1;
> }
> +
> /* setting stack that it would contain only
> * the function pointer. */
> if (index != LUA_GLOBALSINDEX) {
> if (objstack == 0) { /* no object, only a function */
> - lua_replace(L, 1);
> + lua_replace(L, top + 1);
> + lua_pop(L, lua_gettop(L) - top - 1);
> } else if (objstack == 1) { /* just two values, swap them */
> lua_insert(L, -2);
> + lua_pop(L, lua_gettop(L) - top - 2);
> } else { /* long path */
> - lua_insert(L, 1);
> - lua_insert(L, 2);
> + lua_insert(L, top + 1);
> + lua_insert(L, top + 2);
> + lua_pop(L, objstack - 1);
> objstack = 1;
> }
> - lua_settop(L, 1 + objstack);
> }
> - return 1 + objstack;
> + *count = 1 + objstack;
> + return 0;
Why change the return value? We can return -1 on error, >= 0 on success.
IMHO better than returning the value by pointer.
More information about the Tarantool-patches
mailing list