From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 4 Apr 2019 14:17:45 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module Message-ID: <20190404111743.i32fsgmin3uugi5g@tkn_work_nb> References: <20190328020146.lluz4mg5tacpghwv@tkn_work_nb> <35ed4661-9789-7cf1-6627-2ced2a821939@tarantool.org> <6d915212-e80f-4a6d-d884-b838bf25f8a7@tarantool.org> <20190328112158.kpxsk6b55noicbes@tkn_work_nb> <20190403111003.x7vq7olda55tthgi@esperanza> <20190404050733.2xuobbezfzbs47l4@tkn_work_nb> <20190404083823.adyv5zxrlgobfro2@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190404083823.adyv5zxrlgobfro2@esperanza> To: Vladimir Davydov Cc: Kirill Shcherbatov , tarantool-patches@freelists.org List-ID: On Thu, Apr 04, 2019 at 11:38:24AM +0300, Vladimir Davydov wrote: > On Thu, Apr 04, 2019 at 08:07:34AM +0300, Alexander Turenko wrote: > > > Also, key_def.compare() sounds like it compares key definitions, not > > > tuples. May be, we should move these functions to box.tuple module? > > > > I'm tentative about that. The key_def Lua module is planned to be the > > interface to comparators and here we're using comparators. I don't like > > spreading of such function across several modules. Maybe 'key_def' name > > is not good and we need to use something dedicated from the word > > 'comparator'? > > It's not just a comparator. It's also used for extracting keys. > Regarding moving this code to box.tuple - I don't insist. Let's > just solicit approval from Kostja on this one. It is a comparator except extracting keys. Or keys-related functions except comparing a tuple against a tuple. Now I don't sure. > > > > > > > > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better > > > introduce 'equal', 'greater', 'less', etc helpers returning bool? > > > > A function for table.sort(table, func) returns boolean, so it make > > sense. I'm a bit afraid that we'll need to make two calls: say, :less() > > and :equal() to determine an order of tuples strictly. But I cannot > > provide a case where it can be necessary. Are you know one? > > No, I don't write much code in Lua. > > However, if we decided to switch to bool return parameter, I'd implement > all possible combinations, i.e. eq, ge, le, gt, lt. I mean are there cases when we need to check, say t1 < t2 and if it is false then check whether t1 == t2 or t1 > t2? In other words, cases when we need to distinguish all three possible situations. Some algorithms? I looked throught [1] and it seems it does not contain any reason why <=> operator was added to C++20 except ones related to comparison operations autogeneration. We can just add :cmp() in addition to all other variants, what do you think? We can also do the trick: return {eq = } from :le() / :ge() instead of true, but this way looks weird. [1]: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0515r0.pdf > > > > +LUA_API int > > > > +luaopen_key_def(struct lua_State *L) > > > > +{ > > > > + luaL_cdef(L, "struct key_def;"); > > > > + key_def_type_id = luaL_ctypeid(L, "struct key_def&"); > > > > + > > > > + /* Export C functions to Lua. */ > > > > + static const struct luaL_Reg meta[] = { > > > > + {"new", lbox_key_def_new}, > > > > + {NULL, NULL} > > > > + }; > > > > + luaL_register_module(L, "key_def", meta); > > > > + > > > > + lua_newtable(L); /* key_def.internal */ > > > > + lua_pushcfunction(L, lbox_key_def_extract_key); > > > > + lua_setfield(L, -2, "extract_key"); > > > > + lua_pushcfunction(L, lbox_key_def_compare); > > > > + lua_setfield(L, -2, "compare"); > > > > + lua_pushcfunction(L, lbox_key_def_compare_with_key); > > > > + lua_setfield(L, -2, "compare_with_key"); > > > > + lua_pushcfunction(L, lbox_key_def_merge); > > > > + lua_setfield(L, -2, "merge"); > > > > + lua_pushcfunction(L, lbox_key_def_to_table); > > > > + lua_setfield(L, -2, "totable"); > > > > + lua_setfield(L, -2, "internal"); > > > > > > Why 'internal'? We use them as is as key_def methods. > > > E.g. box.tuple.* methods aren't internal. > > > > To distinguish between module and instance methods and don't confuse a > > But 'compare', 'merge', etc can be used both as module methods and as > instance method, i.e. neither of the ways of using say compare is wrong: > > k = key_def.new(...) > k:compare(t1, t2) > key_def.compare(k, t1, t2) > > > user with, say, tab completion in a console. fio.c does the same. > > I like to see all public methods suggested to me when I type > key_def.. Don't know about fio, but box.tuple does the same: > > t:update(...) > box.tuple.update(t, ...) > > are equally correct AFAIU. Hm. I didn't thought about that in this way. Okay. > > > However using, say, :map(box.tuple.totable) is > > convenient, so maybe it worth to name this table, say, > > 'key_def.instance'?