From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 1 Mar 2019 07:08:52 +0300 From: Alexander Turenko Subject: Re: [PATCH v2 3/6] lua: add luaT_newtuple() Message-ID: <20190301040851.vz56hdl4cogt3jjg@tkn_work_nb> References: <20190110124446.7jjzs5nbl34atezq@esperanza> <20190118215820.royn7thqv4xuflb4@tkn_work_nb> <20190123161226.fbarwvyjpcecg2hl@esperanza> <20190128165129.zyy7lyhrstnwnzk5@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190128165129.zyy7lyhrstnwnzk5@tkn_work_nb> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: I made two minor updates to the commit. I pasted them below separatelly for ease reading, but I'll squash them into the original commit. WBR, Alexander Turenko. commit 819687fc2a87f96c2c9445b2476dea2a5d140e01 Author: Alexander Turenko Date: Thu Feb 21 18:37:58 2019 +0300 FIXUP: luaT_tuple_new(): fix test comment diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c index 582d6c616..e424d50bd 100644 --- a/test/unit/luaT_tuple_new.c +++ b/test/unit/luaT_tuple_new.c @@ -9,7 +9,7 @@ #include "box/box.h" /* box_init() */ #include "box/tuple.h" /* box_tuple_format_default() */ #include "lua/msgpack.h" /* luaopen_msgpack() */ -#include "box/lua/tuple.h" /* luaL_iterator_*() */ +#include "box/lua/tuple.h" /* luaT_tuple_new() */ #include "diag.h" /* struct error, diag_*() */ #include "exception.h" /* type_IllegalParams */ commit 5f3cefa306c43308ebc06f9b50a2e26684841746 Author: Alexander Turenko Date: Fri Mar 1 02:28:05 2019 +0300 FIXUP: luaT_tuple_new: after rebase fix diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c index e424d50bd..bcb6347be 100644 --- a/test/unit/luaT_tuple_new.c +++ b/test/unit/luaT_tuple_new.c @@ -122,6 +122,7 @@ test_basic(struct lua_State *L) part.is_nullable = false; part.nullable_action = ON_CONFLICT_ACTION_DEFAULT; part.sort_order = SORT_ORDER_ASC; + part.path = NULL; struct key_def *key_def = key_def_new(&part, 1); box_tuple_format_t *another_format = box_tuple_format_new(&key_def, 1); key_def_delete(key_def); On Mon, Jan 28, 2019 at 07:51:29PM +0300, Alexander Turenko wrote: > Comments are below, diff is at the bottom of the email. > > > Let's just call this function luaT_tuple_new ;-) > > Ok. > > > > > > > > > > > > > > > +{ > > > > > + 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()? > > It is possible, but we need to call next() (and, then, luaT_tuple_new()) > from C part of merger, so API of those functions should not use a lua > stack for errors. So I'll leave diag_set() for errors in > luaT_tuple_new(). > > BTW, we'll need to introduce a flag in merger_state (states that sources > was not initialized yet) to roll a loop with inserts into the heap. > > > > > > 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)? > > Looks okay. Thanks! > > > > > > > > > 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. > > I'll use diag_set(). > > ---- > > (Below are resulting changes, but I amended them into 'lua: add > luaT_tuple_new()' and 'lua: optimize creation of a tuple from a tuple' > commits as approariate.) > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index ab861c6d2..c4f323717 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -122,18 +122,16 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format) > 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())); > + if (tuple == NULL) > return NULL; > - } > ibuf_reinit(tarantool_lua_ibuf); > return tuple; > } > > tuple = luaT_istuple(L, idx); > if (tuple == NULL) { > - lua_pushfstring(L, "A tuple or a table expected, got %s", > - lua_typename(L, lua_type(L, idx))); > + diag_set(IllegalParams, "A tuple or a table expected, got %s", > + lua_typename(L, lua_type(L, idx))); > return NULL; > } > > @@ -144,10 +142,8 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format) > const char *tuple_beg = tuple_data(tuple); > const char *tuple_end = tuple_beg + tuple->bsize; > tuple = box_tuple_new(format, tuple_beg, tuple_end); > - if (tuple == NULL) { > - luaT_pusherror(L, diag_last_error(diag_get())); > + if (tuple == NULL) > return NULL; > - } > return tuple; > } > > @@ -169,7 +165,7 @@ lbox_tuple_new(lua_State *L) > box_tuple_format_t *fmt = box_tuple_format_default(); > struct tuple *tuple = luaT_tuple_new(L, idx, fmt); > if (tuple == NULL) > - return lua_error(L); > + return luaT_error(L); > /* box_tuple_new() doesn't leak on exception, see public API doc */ > luaT_pushtuple(L, tuple); > return 1; > diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h > index f8c8ccf1c..06efa277a 100644 > --- a/src/box/lua/tuple.h > +++ b/src/box/lua/tuple.h > @@ -75,8 +75,7 @@ luaT_istuple(struct lua_State *L, int idx); > * Set idx to zero to create the new tuple from objects on the lua > * stack. > * > - * In case of an error push the error message to the Lua stack and > - * return NULL. > + * In case of an error set diag and return NULL. > */ > struct tuple * > luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format); > diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c > index 07fa1a792..582d6c616 100644 > --- a/test/unit/luaT_tuple_new.c > +++ b/test/unit/luaT_tuple_new.c > @@ -10,6 +10,8 @@ > #include "box/tuple.h" /* box_tuple_format_default() */ > #include "lua/msgpack.h" /* luaopen_msgpack() */ > #include "box/lua/tuple.h" /* luaL_iterator_*() */ > +#include "diag.h" /* struct error, diag_*() */ > +#include "exception.h" /* type_IllegalParams */ > > /* > * This test checks all usage cases of luaT_tuple_new(): > @@ -52,10 +54,10 @@ void check_error(struct lua_State *L, const struct tuple *tuple, int retvals, > { > const char *exp_err = "A tuple or a table expected, got number"; > is(tuple, NULL, "%s: tuple == NULL", case_name); > - is(retvals, 1, "%s: check retvals count", case_name); > - is(lua_type(L, -1), LUA_TSTRING, "%s: check error type", case_name); > - ok(!strcmp(lua_tostring(L, -1), exp_err), "%s: check error message", > - case_name); > + is(retvals, 0, "%s: check retvals count", case_name); > + struct error *e = diag_last_error(diag_get()); > + is(e->type, &type_IllegalParams, "%s: check error type", case_name); > + ok(!strcmp(e->errmsg, exp_err), "%s: check error message", case_name); > } > > int > @@ -147,7 +149,7 @@ test_basic(struct lua_State *L) > check_error(L, tuple, lua_gettop(L) - top, "unexpected type"); > > /* Clean up. */ > - lua_pop(L, 2); > + lua_pop(L, 1); > assert(lua_gettop(L) == 0); > > footer();