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