Re: [PATCH v3 5/6] box: refactor box_lua_find helper

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

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.