From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C5C5420CE3 for ; Mon, 23 Jul 2018 18:38:28 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x2EUKETWKVB6 for ; Mon, 23 Jul 2018 18:38:28 -0400 (EDT) Received: from forward5p.cmail.yandex.net (forward5p.cmail.yandex.net [77.88.31.20]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0F92B20C58 for ; Mon, 23 Jul 2018 18:38:27 -0400 (EDT) From: roman.habibov1@yandex.ru In-Reply-To: <12c20a86-8a40-4fe6-8dab-7b7b9494a142@tarantool.org> References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> <12c20a86-8a40-4fe6-8dab-7b7b9494a142@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() MIME-Version: 1.0 Date: Tue, 24 Jul 2018 01:38:24 +0300 Message-Id: <39430371532385504@iva8-37fc2ad204cd.qloud-c.yandex.net> Content-Transfer-Encoding: 7bit Content-Type: text/plain Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , "tarantool-patches@freelists.org" 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.