[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()

[roman.habibov1@yandex.ru]

Hi, thanks for your review.

Branch: https://github.com/tarantool/tarantool/tree/romankhabibov/gh-2888-options-to-encode

Issue: https://github.com/tarantool/tarantool/issues/2888


> 1. You should not touch this hunk. Utils should provide
> only API to configure the serializer, and do not expose
> options array.
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 0626bf76b..012758b7f 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -186,19 +186,35 @@ luaL_setcdatagc(struct lua_State *L, int idx)
 	lua_pop(L, 1);
 }
 
+#define OPTION(type, name, defvalue) { #name, \
+	offsetof(struct luaL_serializer, name), type, defvalue}
 /**
- * @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.
- * @param L lua stack
- * @return 0
+ * Configuration options for serializers
+ * @sa struct luaL_serializer
  */
-
-/*Serialize one option. Returns ponter to the value of option.*/
-int* parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {
+static struct {
+	const char *name;
+	size_t offset; /* offset in structure */
+	int type;
+	int defvalue;
+} OPTIONS[] = {
+	OPTION(LUA_TBOOLEAN, encode_sparse_convert, 1),
+	OPTION(LUA_TNUMBER,  encode_sparse_ratio, 2),
+	OPTION(LUA_TNUMBER,  encode_sparse_safe, 10),
+	OPTION(LUA_TNUMBER,  encode_max_depth, 32),
+	OPTION(LUA_TBOOLEAN, encode_invalid_numbers, 1),
+	OPTION(LUA_TNUMBER,  encode_number_precision, 14),
+	OPTION(LUA_TBOOLEAN, encode_load_metatables, 1),
+	OPTION(LUA_TBOOLEAN, encode_use_tostring, 0),
+	OPTION(LUA_TBOOLEAN, encode_invalid_as_nil, 0),
+	OPTION(LUA_TBOOLEAN, decode_invalid_numbers, 1),
+	OPTION(LUA_TBOOLEAN, decode_save_metatables, 1),
+	OPTION(LUA_TNUMBER,  decode_max_depth, 32),
+	{ NULL, 0, 0, 0},
+};

> 2. You've cut the comment off. It belongs to another function.
Fixed.

> 3. Bad style. Please, see in other functions how to write comments
> and how to declare pointers. You should not write int*, but int *.
> Please, wrap line after the function return type declaring.
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 4a2c3eaac..21eb36856 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
 /**
- * Configuration options for serializers
- * @sa struct luaL_serializer
+ * parse_option is a function that is used to configure one field
+ * in luaL_serializer struct. Adds one lua table to the top of 
+ * Lua stack.
+ * @param L lua stack
+ * @param index of option in OPTIONS[]
+ * @param serializer to inherit configuration
+ * @return ponter to the value of option
  */
+int *
+parse_option(lua_State *l, int i, struct luaL_serializer* cfg);
 
-int* parse_option(lua_State *l, int i, struct luaL_serializer* cfg);
+/**
+ * parse_options is a function that is used to serialize lua table
+ * of options to luaL_serializer struct. Removes the lua table from
+ * the top of lua stack.
+ * parse_options.
+ * @param L lua stack
+ * @param serializer to inherit configuration
+ * @return 0
+ */
+int
+parse_options(lua_State *l, struct luaL_serializer* cfg);

> 4. Broken indentation.
Fixed.

> 5. Garbage diff. You should never change compilation
> options via pragma. And if you need this one, then your
> patch has a problem, that should be fixed, not hidden.
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 4a2c3eaac..21eb36856 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -1,6 +1,5 @@
 #ifndef TARANTOOL_LUA_UTILS_H_INCLUDED
 #define TARANTOOL_LUA_UTILS_H_INCLUDED
-#pragma GCC diagnostic ignored "-Wunused-variable"

> 6. Please, write function comment on top of the declaration. Not on
> top of the implementation.
Fixed.

> 7. Typo: opions.
Fixed.

> 8. Please, consult other test files how to write a test comment.
> It should not be a description of what was done, but an explanation
> of the problem.
> 9. I do not see tests on decode.
diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
index 050d769ea..d76297ab5 100755
--- a/test/app-tap/json.test.lua
+++ b/test/app-tap/json.test.lua
@@ -22,42 +22,55 @@ end
 
 tap.test("json", function(test)
     local serializer = require('json')
-    test:plan(18)
+    test:plan(21)
 
--- gh-2888 Added opions to encode().
+-- gh-2888: check the possibility of using options in encode()/decode()
+    serializer.cfg({decode_invalid_numbers = false})
+    test:ok(pcall(serializer.decode, '{"a":inf}') == false,
+        'expected error with NaN decoding with .cfg')
+    serializer.cfg({decode_invalid_numbers = true})
+    test:ok(pcall(serializer.decode, '{"a":inf}',
+        {decode_invalid_numbers = false}) == false,
+        'expected error with NaN decoding with .decode')
+
+    test:ok(pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}',
+        {decode_max_depth = 2}) == false,
+        'error: too many nested data structures')
+
+--



> 10. Bad indentation. In decode too.
Fixed.