From: Alexander Turenko <alexander.turenko@tarantool.org> To: Vladimir Davydov <vdavydov.dev@gmail.com> Cc: tarantool-patches@freelists.org Subject: Re: [PATCH v2 3/6] lua: add luaT_newtuple() Date: Fri, 1 Mar 2019 07:08:52 +0300 [thread overview] Message-ID: <20190301040851.vz56hdl4cogt3jjg@tkn_work_nb> (raw) In-Reply-To: <20190128165129.zyy7lyhrstnwnzk5@tkn_work_nb> 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 <alexander.turenko@tarantool.org> 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 <alexander.turenko@tarantool.org> 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();
next prev parent reply other threads:[~2019-03-01 4:08 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-09 20:20 [PATCH v2 0/6] Merger Alexander Turenko 2019-01-09 20:20 ` [PATCH v2 1/6] Add luaL_iscallable with support of cdata metatype Alexander Turenko 2019-01-10 12:21 ` Vladimir Davydov 2019-01-09 20:20 ` [PATCH v2 2/6] Add functions to ease using Lua iterators from C Alexander Turenko 2019-01-10 12:29 ` Vladimir Davydov 2019-01-15 23:26 ` Alexander Turenko 2019-01-16 8:18 ` Vladimir Davydov 2019-01-16 11:40 ` Alexander Turenko 2019-01-16 12:20 ` Vladimir Davydov 2019-01-17 1:20 ` Alexander Turenko 2019-01-28 18:17 ` Alexander Turenko 2019-03-01 4:04 ` Alexander Turenko 2019-01-09 20:20 ` [PATCH v2 3/6] lua: add luaT_newtuple() Alexander Turenko 2019-01-10 12:44 ` Vladimir Davydov 2019-01-18 21:58 ` Alexander Turenko 2019-01-23 16:12 ` Vladimir Davydov 2019-01-28 16:51 ` Alexander Turenko 2019-03-01 4:08 ` Alexander Turenko [this message] 2019-01-09 20:20 ` [PATCH v2 4/6] lua: add luaT_new_key_def() Alexander Turenko 2019-01-10 13:07 ` Vladimir Davydov 2019-01-29 18:52 ` Alexander Turenko 2019-01-30 10:58 ` Alexander Turenko 2019-03-01 4:10 ` Alexander Turenko 2019-01-09 20:20 ` [PATCH v2 5/6] net.box: add helpers to decode msgpack headers Alexander Turenko 2019-01-10 17:29 ` Vladimir Davydov 2019-02-01 15:11 ` Alexander Turenko 2019-03-05 12:00 ` Alexander Turenko 2019-01-09 20:20 ` [PATCH v2 6/6] Add merger for tuple streams Alexander Turenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190301040851.vz56hdl4cogt3jjg@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [PATCH v2 3/6] lua: add luaT_newtuple()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox