[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()
- From: roman.habibov1@xxxxxxxxx
- To: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>, "tarantool-patches@xxxxxxxxxxxxx" <tarantool-patches@xxxxxxxxxxxxx>
- Date: Tue, 24 Jul 2018 01:38:24 +0300
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.
Other related posts:
