From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Jun 2019 17:22:04 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 5/6] box: refactor box_lua_find helper Message-ID: <20190618142204.enm7ey52ts6djpx3@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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.