[tarantool-patches] Re: [PATCH v2 3/4] tuple: use global msgpack serializer in Lua tuple

[Vladislav Shpilevoy]

Thanks for the review!

On 13/09/2019 01:27, Alexander Turenko wrote:
> LGTM.
> 
> Several questions are below.
> 
> I agree that the variant with triggers looks more natural.
> 
> Should not we add box_lua_tuple_free() and call trigger_destroy()
> inside? I know, we now don't call tarantool_lua_free(), but I hope it
> will be fixed someday.
> 
> If you'll going to add it, please, add also the call to luaT_tuple_new.c
> for the symmetry with box_lua_tuple_init(). It also will help to keep
> this test clean from ASAN / Valgrind point of view: I did verify it
> aganst one of those tools at the time of writing the test (don't
> remember against which of them).

I tried to add box_lua_tuple_free, but as you mentioned, we don't
have box_lua_free() so that function is unused. I don't think it
should be added now.

Also I stumbled into a problem, that luaT_tuple_new.c uses
luaopen_msgpack, but we don't have luaclose_msgpack. So there
are 2 holes. Moreover, as I understand, no one of our Lua C
modules really drops its bindings from Lua namespace. It is
not related to this issue, but I remember that someday we wanted
to be able to 'uncfg' box, like it was not called.

> 
> Hm. We don't have box_lua_free() at all. I'm doubtful now.
> 
> WBR, Alexander Turenko.
> 
>> +static inline void
>> +tuple_serializer_fill(void)
>> +{
>> +	luaL_serializer_copy_options(&tuple_serializer, luaL_msgpack_default);
>> +	tuple_serializer.encode_sparse_ratio = 0;
>> +}
> 
> Is not this name quite common? Maybe tuple_serializer_update_options()?
> 

Honestly, I don't really care that much. The function with all its invocations
fits in one screen. Both names are ok for me:

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index fc22f5572..8b59466b9 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -554,7 +554,7 @@ static const struct luaL_Reg lbox_tuple_iterator_meta[] = {
 /* }}} */
 
 static inline void
-tuple_serializer_fill(void)
+tuple_serializer_update_options(void)
 {
 	luaL_serializer_copy_options(&tuple_serializer, luaL_msgpack_default);
 	tuple_serializer.encode_sparse_ratio = 0;
@@ -565,7 +565,7 @@ on_msgpack_serializer_update(struct trigger *trigger, void *event)
 {
 	(void) trigger;
 	(void) event;
-	tuple_serializer_fill();
+	tuple_serializer_update_options();
 }
 
 void
@@ -584,7 +584,7 @@ box_lua_tuple_init(struct lua_State *L)
 
 	luamp_set_encode_extension(luamp_encode_extension_box);
 
-	tuple_serializer_fill();
+	tuple_serializer_update_options();
 	trigger_create(&tuple_serializer.update_trigger,
 		       on_msgpack_serializer_update, NULL, NULL);
 	trigger_add(&luaL_msgpack_default->on_update,