Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Cyrill Gorcunov <gorcunov@gmail.com>]

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 ?
> > +		"<unknown>" : 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