[tarantool-patches] Re: [PATCH v2 1/4] app: serializers update now is reflected in Lua

[Vladislav Shpilevoy]

Thanks for the review!

On 13/09/2019 01:22, Alexander Turenko wrote:
> LGTM.
> 
> Small comments are below.
> 
> WBR, Alexander Turenko.
> 
>>  src/lua/utils.c                      | 34 +++++++++++++++-------------
>>  test/app-tap/json.test.lua           |  3 ++-
>>  test/app-tap/lua/serializer_test.lua | 13 +++++++++++
>>  test/app-tap/msgpack.test.lua        |  3 ++-
> 
> It looks that it worth to add the test to test/app-tap/yaml.test.lua
> too.

The test is called 'test_depth' and YAML serializer does not check max
depth. This is why I omitted it for YAML, deliberately. Next commits
add here more depth-specific tests, and they will fail for YAML.

>>  4 files changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/lua/utils.c b/src/lua/utils.c
>> index 75efe0ed2..a082a2e5b 100644
>> --- a/src/lua/utils.c
>> +++ b/src/lua/utils.c
>> @@ -270,10 +270,8 @@ luaL_serializer_parse_option(struct lua_State *L, int i,
>>  			     struct luaL_serializer *cfg)
> 
> Now the function pushes a value to a Lua stack, I think it worth to
> update the doxygen-style comment.
> 

I thought it is quite obvious, and the old comment was still valid. But
ok:

diff --git a/src/lua/utils.c b/src/lua/utils.c
index a082a2e5b..6789a8477 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -258,7 +258,8 @@ luaL_serializer_create(struct luaL_serializer *cfg)
 }
 
 /**
- * Configure one field in @a cfg.
+ * Configure one field in @a cfg. Value of the field is kept on
+ * Lua stack after this function, and should be popped manually.
  * @param L Lua stack.
  * @param i Index of option in OPTIONS[].
  * @param cfg Serializer to inherit configuration.

(I didn't describe it in @retval, because this C function does not
return Lua values. It still returns the same - int pointer).

>>  /**
>> - * @brief serializer.cfg{} Lua binding for serializers.
>> - * serializer.cfg is a table that contains current configuration values from
>> - * luaL_serializer structure. serializer.cfg has overriden __call() method
>> - * to change configuration keys in internal userdata (like box.cfg{}).
>> - * Please note that direct change in serializer.cfg.key will not affect
>> - * internal state of userdata.
> 
> The note is still actual, I would not remove it.

The old comment is super overcomplicated and obvious I think. But ok:

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 6789a8477..93e00d81f 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -302,9 +302,15 @@ luaL_serializer_parse_options(struct lua_State *L,
 }
 
 /**
- * Serializer.cfg.__call implementation. Merge new parameters into
- * both C structure of the serializer, and into its Lua table
- * representation: serializer.cfg.
+ * @brief serializer.cfg{} Lua binding for serializers.
+ * serializer.cfg is a table that contains current configuration values from
+ * luaL_serializer structure. serializer.cfg has overriden __call() method
+ * to change configuration keys in internal userdata (like box.cfg{}).
+ * Please note that direct change in serializer.cfg.key will not affect
+ * internal state of userdata. Changes via cfg() are reflected in
+ * both Lua cfg table, and C serializer structure.
+ * @param L lua stack
+ * @return 0
  */
 static int
 luaL_serializer_cfg(struct lua_State *L)

> 
> ( Sooner or later we should just do
> https://github.com/tarantool/tarantool/issues/2867 )
> 
> Side note: I think that it is better to have one source of information
> and have only a handle in Lua. Or maybe (if option(s) access cost is
> matter) update the Lua table using some kind of triggers: if we'll
> change an option from C, then the current implementation will fail to
> track it.

We could copy luaL_serializer to ffi.cdef so as Lua would be able to work
with that structure directly. That looks like a piece of cake.

> 
> Anyway, it is not a part of your issue. Just side thoughts.
>