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 E92CE28F1F for ; Thu, 2 Aug 2018 08:59:15 -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 g7_7kS1LySHq for ; Thu, 2 Aug 2018 08:59:15 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 193FD28F19 for ; Thu, 2 Aug 2018 08:59:15 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> <12c20a86-8a40-4fe6-8dab-7b7b9494a142@tarantool.org> <39430371532385504@iva8-37fc2ad204cd.qloud-c.yandex.net> <3367531532598057@sas1-4b7566131ec9.qloud-c.yandex.net> <0e2c47c2-0276-a1da-c54b-726b64c17aae@tarantool.org> <5209131532608150@myt3-c7e5d17fe013.qloud-c.yandex.net> <2952461532611162@iva5-08e20335b0d9.qloud-c.yandex.net> <33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org> <28658371533050946@myt4-74a8acfc13eb.qloud-c.yandex.net> <898281533156060@myt5-cf6d29327892.qloud-c.yandex.net> From: Vladislav Shpilevoy Message-ID: Date: Thu, 2 Aug 2018 15:59:11 +0300 MIME-Version: 1.0 In-Reply-To: <898281533156060@myt5-cf6d29327892.qloud-c.yandex.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: roman.habibov1@yandex.ru, "tarantool-patches@freelists.org" Hi! Thanks for the fixes! See 10 comments below. 1. Put the branch onto 1.10, not on 2.0. It does not require SQL, so 1.10 is ok. > commit 1ecdfa05d8aa0037da79eeeba19cecb06bab2103 > Author: Roman Khabibov > Date: Sun Jul 8 02:21:08 2018 +0300 > > json: add options to json.encode() > > Add an ability to pass options to json.encode()/decode(). > > Closes: #2888. > > diff --git a/src/lua/utils.c b/src/lua/utils.c > index 2f0f4dcf8..e200fee0a 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -214,6 +214,47 @@ static struct { > { NULL, 0, 0, 0}, > }; > > +/** > + * Configure one field in @a cfg. > + * @param L lua stack > + * @param i index of option in OPTIONS[] > + * @param cfg serializer to inherit configuration > + * @retval pointer to the value of option, NULL if option is not > + * in the table 2. As I said earlier, please, use separate @retval for each possible value. One @retval for not NULL pointer, one @retval for NULL. > + */ > +static int * > +luaL_serializer_parse_option(struct lua_State *L, > + int i, struct luaL_serializer *cfg) 3. Both here and in luaL_serializer_parse_options indentation is broken after you changed the functions names. > +{ > + lua_getfield(L, 2, OPTIONS[i].name); > + if (lua_isnil(L, -1)) { > + lua_pop(L, 1); > + return NULL; > + } > + int *pval = (int *) ((char *) cfg + OPTIONS[i].offset); > + switch (OPTIONS[i].type) { > + case LUA_TBOOLEAN: > + *pval = lua_toboolean(L, -1); > + break; > + case LUA_TNUMBER: > + *pval = lua_tointeger(L, -1); > + break; > + default: > + unreachable(); > + } > + lua_pop(L, 1); > + return pval; > +} > + > +void > +luaL_serializer_parse_options(struct lua_State *L, > + struct luaL_serializer *cfg) > +{ > + for (int i = 0; OPTIONS[i].name != NULL; i++) > + luaL_serializer_parse_option(L, i, cfg); > + lua_pop(L, 1); > +} > + > /** > * @brief serializer.cfg{} Lua binding for serializers. > * serializer.cfg is a table that contains current configuration values from > @@ -225,38 +266,29 @@ static struct { > * @return 0 > */ > static int > -luaL_serializer_cfg(lua_State *L) > +luaL_serializer_cfg(struct lua_State *L) > { > luaL_checktype(L, 1, LUA_TTABLE); /* serializer */ > luaL_checktype(L, 2, LUA_TTABLE); /* serializer.cfg */ > struct luaL_serializer *cfg = luaL_checkserializer(L); > - /* Iterate over all available options and checks keys in passed table */ > + /* Iterate over all available options and checks keys in passed table. */ > for (int i = 0; OPTIONS[i].name != NULL; i++) { > - lua_getfield(L, 2, OPTIONS[i].name); > - if (lua_isnil(L, -1)) { > - lua_pop(L, 1); /* key hasn't changed */ > - continue; > - } > - /* > - * Update struct luaL_serializer using pointer to a > - * configuration value (all values must be `int` for that). > - */ > - int *pval = (int *) ((char *) cfg + OPTIONS[i].offset); > - /* Update struct luaL_serializer structure */ > - switch (OPTIONS[i].type) { > - case LUA_TBOOLEAN: > - *pval = lua_toboolean(L, -1); > - lua_pushboolean(L, *pval); > - break; > - case LUA_TNUMBER: > - *pval = lua_tointeger(L, -1); > - lua_pushinteger(L, *pval); > - break; > - default: > - unreachable(); > + int *pval = luaL_serializer_parse_option(L, i, cfg); 4. I've noticed that all the code below (push, setfield) is useless. parse_option gets the value from cfg[name] and sets it to struct luaL_cfg_serializer.name. Then the same value is pushed back onto the stack and set into cfg[name]. So actually it looks like cfg[name] = cfg[name]. I've removed the code below and all works ok. Please, do it. > + /* Update struct luaL_serializer structure. */ > + if (pval != NULL) { > + switch (OPTIONS[i].type) { > + case LUA_TBOOLEAN: > + lua_pushboolean(L, *pval); > + break; > + case LUA_TNUMBER: > + lua_pushinteger(L, *pval); > + break; > + default: > + unreachable(); > + } > + /* Save normalized value to serializer.cfg table. */ > + lua_setfield(L, 1, OPTIONS[i].name); > } > - /* Save normalized value to serializer.cfg table */ > - lua_setfield(L, 1, OPTIONS[i].name); > } > return 0; > } > diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua > index 3884b41e7..2a219ec24 100755 > --- a/test/app-tap/json.test.lua > +++ b/test/app-tap/json.test.lua > @@ -22,7 +22,55 @@ end > > tap.test("json", function(test) > local serializer = require('json') > - test:plan(9) > + test:plan(21) > + > +-- gh-2888: Check the possibility of using options in encode()/decode(). 5. Please, align function's lines by the function's body offset. Including comments. > + > + local sub = {a = 1, { b = {c = 1, d = {e = 1}}}} > + serializer.cfg({encode_max_depth = 1}) > + test:ok(serializer.encode(sub) == '{"1":null,"a":1}', > + 'depth of encoding is 1 with .cfg') > + serializer.cfg({encode_max_depth = 2}) > + test:ok(serializer.encode(sub) == '{"1":{"b":null},"a":1}', > + 'depth of encoding is 2 with .cfg') > + serializer.cfg({encode_max_depth = 2}) 6. You've called the same cfg 2 lines above. > + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}', > + 'depth of encoding is 1 with .encode') 7. Please, align function arguments under the function beginning. I've pushed several fixes of this violation on a separate commit on your branch. Please, look at them, squash, and do the same alignment in other places in this file and other files. > + > + local nan = 1/0 > + test:ok(serializer.encode({a = nan}) == '{"a":inf}', > + 'default "encode_invalid_numbers"') > + serializer.cfg({encode_invalid_numbers = false}) > + test:ok(pcall(serializer.encode, {a = nan}) == false, 8. I believe, instead of 'boolean == false' you can just do 'not boolean' (I've fixed it here. Do the same in other places). > + 'expected error with NaN ecoding with .cfg') > + serializer.cfg({encode_invalid_numbers = true}) > + test:ok(pcall(serializer.encode, {a = nan}, > + {encode_invalid_numbers = false}) == false, > + 'expected error with NaN ecoding with .encode') > + > + local number = 0.12345 > + test:ok(serializer.encode({a = number}) == '{"a":0.12345}', > + 'precision more than 5') > + serializer.cfg({encode_number_precision = 3}) > + test:ok(serializer.encode({a = number}) == '{"a":0.123}', > + 'precision is 3') > + serializer.cfg({encode_number_precision = 14}) > + test:ok(serializer.encode({a = number}, > + {encode_number_precision = 3}) == '{"a":0.123}', 'precision is 3') > + > + 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') > + > +-- 9. Garbage empty comment. > test:test("unsigned", common.test_unsigned, serializer) > test:test("signed", common.test_signed, serializer) > test:test("double", common.test_double, serializer) > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c > index aa8217dfb..1a1bb3180 100644 > --- a/third_party/lua-cjson/lua_cjson.c > +++ b/third_party/lua-cjson/lua_cjson.c > @@ -417,22 +417,25 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg, > } > } > > -static int json_encode(lua_State *l) > -{ > - struct luaL_serializer *cfg = luaL_checkserializer(l); > - char *json; > - int len; > +static int json_encode(lua_State *l) { > + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), > + 1, "expected 1 or 2 arguments"); > > - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); > - > - /* Reuse existing buffer */ > + /* Reuse existing buffer. */ > strbuf_reset(&encode_buf); > + struct luaL_serializer *cfg = luaL_checkserializer(l); > > - json_append_data(l, cfg, 0, &encode_buf); > - json = strbuf_string(&encode_buf, &len); > + if (lua_gettop(l) == 2) { > + struct luaL_serializer user_cfg = *cfg; > + luaL_serializer_parse_options(l, &user_cfg); > + json_append_data(l, &user_cfg, 0, &encode_buf); > + } else { > + json_append_data(l, cfg, 0, &encode_buf); > + } > > + int len; > + char *json = strbuf_string(&encode_buf, &len); 10. 'len' parameter is ok to be NULL here. > lua_pushlstring(l, json, len); > - > return 1; > }