Re: [PATCH v2 3/6] lua: add luaT_newtuple()

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

On Sat, Jan 19, 2019 at 12:58:20AM +0300, Alexander Turenko wrote:
> > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> > > index 1867f810f..7e9ad89fe 100644
> > > --- a/src/box/lua/tuple.c
> > > +++ b/src/box/lua/tuple.c
> > > @@ -92,6 +92,65 @@ luaT_istuple(struct lua_State *L, int narg)
> > >  	return *(struct tuple **) data;
> > >  }
> > >  
> > > +struct tuple *
> > > +luaT_newtuple(struct lua_State *L, int idx, box_tuple_format_t *format)
> > 
> > I looked at the Lua reference manual and realized that they usually call
> > a function lua_newsomething if it creates an object on Lua stack. So I
> > guess we'd better rename it to luaT_tuple_new() to avoid confusion.
> 
> Renamed, but I'm tentative about this change.
> 
> We'll have the following convention:
> 
> * lua{,L,T}_foo_new(): read from a Lua stack, return a pointer, push an
>   error to a Lua stack (errors is the open question, see below);
> * lua{,L,T}_newfoo(): read from a Lua stack, push on a Lua stack, raise
>   an error;
> * lbox_foo_new(): the same as previous, but exposed into Lua via a
>   module / instance fields.
> 
> Don't sure I'll able to remember this convention: a name does not much
> say about a behaviour. I would stick with luaT_new* or introduce some

luaT_newkey_def would look awful, wouldn't it?

> more explicit naming for a function to grab a definition from lua and
> create an object in C.
> 
> Check / to does not fit good:
> 
> * lua{L,T}_checkfoo() -> struct foo *
>   - Like luaL_checkstring().
>   - But it looks as if it can raise an exception.
>   - But it looks as if we'll unwrap a pointer from a cdata.
> * lua{L,T}_tofoo() -> struct foo *
>   - Like lua_tolstring().
>   - But it looks as if we'll unwrap a pointer from a cdata.
> 
> Another possible variants (in order of increasing weirdness for me):
> 
> * lua{L,T}_makefoo();
> * lua{L,T}_createfoo();
> * lua{L,T}_parsefoo();
> * lua{L,T}_newfoo_c();
> * lua{L,T}_constructfoo();
> * lua{L,T}_foo_new_fromlua();
> 
> BTW, '_c' postfix can suggest that a function cannot raise an error.

Better not (why _c?)

> 
> There are lua{,L}_new* functions that returns a pointer:
> 
> * luaL_newstate(void) -> struct lua_State *
> * lua_newthread(struct lua_State *) -> struct lua_State *
> * lua_newuserdata(struct lua_State *, size_t) -> void *

These function don't use Lua stack as input AFAIK so they are different.

> 
> Maybe it is not criminal to do the same under luaT_newtuple()?
> 
> We also can use luaT_create*, I found only one function with such
> naming: lua_createtable().
> 
> I think the general approach could be the following: use
> lua{L,T}_newfoo() or lua{L,T}_createfoo() if the former one is busy.
> 
> luaT_maketuple() and lua{L,T}_newtuple_c() looks appropriate for me too.
> Latter a bit ugly, but '_c' is informative (no exceptions).

I don't know what to say, really. It's overwhelming.

Let's just call this function luaT_tuple_new ;-)

> 
> > 
> > > +{
> > > +	struct tuple *tuple;
> > > +
> > > +	if (idx == 0 || lua_istable(L, idx)) {
> > > +		struct ibuf *buf = tarantool_lua_ibuf;
> > > +		ibuf_reset(buf);
> > > +		struct mpstream stream;
> > > +		mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> > > +		      luamp_error, L);
> > > +		if (idx == 0) {
> > > +			/*
> > > +			 * Create the tuple from lua stack
> > > +			 * objects.
> > > +			 */
> > > +			int argc = lua_gettop(L);
> > > +			mpstream_encode_array(&stream, argc);
> > > +			for (int k = 1; k <= argc; ++k) {
> > > +				luamp_encode(L, luaL_msgpack_default, &stream,
> > > +					     k);
> > > +			}
> > > +		} else {
> > > +			/* Create the tuple from a Lua table. */
> > > +			luamp_encode_tuple(L, luaL_msgpack_default, &stream,
> > > +					   idx);
> > > +		}
> > > +		mpstream_flush(&stream);
> > > +		tuple = box_tuple_new(format, buf->buf,
> > > +				      buf->buf + ibuf_used(buf));
> > > +		if (tuple == NULL) {
> > > +			luaT_pusherror(L, diag_last_error(diag_get()));
> > 
> > Why not simply throw the error with luaT_error()? Other similar
> > functions throw an error, not just push it to the stack.
> 
> Because in a caller I need to perform clean up before reraise it.
> lua_pcall() would be expensive.
> 
> luaT_tuple_new() is used in a table and an iterator source in next().
> next() is used in two places: merger_next() where I just pop and raise
> the error and in merger_source_new() to acquire a first tuple. If an

Why do you need to acquire the first tuple in merger_source_new()?
Can't you do that in next()?

> error occurs here I need to free the newly created source, then in
> merger_state_new() free newly created merger_state with all successfully
> created sources (it also perform needed unref's). I cannot allow any
> function called on this path to raise an error.
> 
> I can implement reference counting of all that objects and free them in
> the next call to merger (some kind of simple gc), but this way looks as
> overengineered.
> 
> Mine errors are strings and it is convenient to create them with
> lua_pushfstring() or push memory errors with luaT_pusherror().
> 
> There are two variants how to avoid raising an error:
> 
> * lua_pushfstring();
> * diag_set().
> 
> Latter seems to be more native for tarantool. I would use something like
> XlogError: printf-style format string + vararg. But I doubt how should I
> name such class? ContractError (most of them are about bad args)? There
> is also unexpected buffer end, it is more RuntimeError.

Use IllegalParams (already exists)?

> 
> I dislike the idea to copy XlogError code under another name. Maybe we
> can implement a general class for such errors and inherit it in
> XlogError, ContractError and RuntimeError?
> 
> I choose pushing to stack, because it is the most simple solution, and
> forget to discuss it with you. My bad.
> 
> Please, give me some pointer here.

I'd prefer to either throw an error (if the merger can handle it) or set
diag to IllegalParams.