From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 23 Jan 2019 19:12:26 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 3/6] lua: add luaT_newtuple() Message-ID: <20190123161226.fbarwvyjpcecg2hl@esperanza> References: <20190110124446.7jjzs5nbl34atezq@esperanza> <20190118215820.royn7thqv4xuflb4@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190118215820.royn7thqv4xuflb4@tkn_work_nb> To: Alexander Turenko Cc: tarantool-patches@freelists.org List-ID: 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.