From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 268CE469719 for ; Mon, 12 Oct 2020 00:58:26 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id a23so14202522ljp.5 for ; Sun, 11 Oct 2020 14:58:26 -0700 (PDT) Date: Mon, 12 Oct 2020 00:58:22 +0300 From: Cyrill Gorcunov Message-ID: <20201011215822.GB14048@grain> References: <20201008213608.1022476-1-gorcunov@gmail.com> <20201008213608.1022476-6-gorcunov@gmail.com> <935104b9-2ba6-6264-ef0b-fa9f587a736d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <935104b9-2ba6-6264-ef0b-fa9f587a736d@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Fri, Oct 09, 2020 at 11:46:20PM +0200, Vladislav Shpilevoy wrote: > > > > `cbox.func.create(name) -> obj | nil, err` > > ------------------------------------------ > > > > Create a new function with name `name`. > > 1. I hope this is a draft. Because in the final version it would be > better to explain what does it mean to 'create' a function (or 'load' > in the next version, I guess). That it will load user's dynamic lib > on the first function call. It does not create anything, just loads > an existing function from a shared file. Yes, this is for review, I'll create more detailed description. > > `cbox.module.reload(name) -> true | nil, err` > > --------------------------------------------- > > > > Reloads a module with name `name` and all functions which > > were associated for the module. > > 2. What happens with the currently working functions? What > happens, if there are still non-deleted refs to the old > functions? What if all refs are deleted, but not on all > functions was called unload()? Old functions remains active until explicitly unloaded. We load symbols with RTLD_LOCAL thus they are not interfere when ld.so resolves the tables. probably i should add more details into documentation. > 3. It seems the function invocation itself is not documented? Good point, seems this would be useful. I though 'cause this is mostly an alias for "_func" space it would be obvious. But indeed without real example the doc looks incomplete. Thanks! > > + > > +#include "tt_static.h" > > 4. Why do you need tt_static.h? You don't use any of tt_* > functions here. Silly me, tnt_errcode_desc doesn't use ststic buffers. Will drop, thanks! > > > +#include "assoc.h" > > +#include "diag.h" > > +#include "port.h" > > +#include "say.h" > > 5. You don't use say_* functions either. I use debug print internally, I dropped them off before this send but I'll add them back on next round. > > + > > +/** > > + * Find function descriptor by its name. > > + */ > > +struct cbox_func * > > +cbox_func_find(const char *name, size_t len) > > 6. I assume the function should be 'static'. It is never > used out of cbox.c. +1 > > +/** > > + * Setup a detailed error description into the diagnostics area. > > + */ > > +static void > > +diag_set_param_error(struct lua_State *L, int idx, const char *func_name, > > + const char *param, const char *exp) > > +{ > > + const char *typename = idx == 0 ? > > + "" : lua_typename(L, lua_type(L, idx)); > > + static const char *fmt = > > + "%s: wrong parameter \"%s\": expected %s, got %s"; > > 7. Please, lets omit 'static' when it is not needed. It just raises questions. Yeah, this one escaped from previous cleanups. > > + > > +/** > > + * Call function by its name from the Lua code. > > + */ > > +static int > > +lcbox_func_call(struct lua_State *L) > > +{ > > + int nr_args = lua_gettop(L); > > 8. It is not really a number of arguments. It includes the function > object itself. Which is an argument. This is just a calling convention. I can rename it to plain "top" if you prefer. > > + > > + /* > > + * FIXME: We should get rid of luaT_newthread but this > > + * requires serious modifucations. In particular > > 9. modifucations -> modifications. thanks! > > + > > + struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val; > > + mh_strnptr_del(func_hash, e, NULL); > > + free(cf); > > 10. I would advise to wrap it into cbox_func_delete(). I am 100% sure > we will add more deletion things to the function object soon. sure