[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()
roman.habibov1 at yandex.ru
roman.habibov1 at yandex.ru
Tue Jul 24 01:38:24 MSK 2018
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.
More information about the Tarantool-patches
mailing list