From: Alexander Turenko <alexander.turenko@tarantool.org> To: Roman Khabibov <roman.habibov1@yandex.ru> Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() Date: Sun, 9 Sep 2018 18:28:04 +0300 [thread overview] Message-ID: <20180909152804.4lagt5drkymriv5x@tkn_work_nb> (raw) In-Reply-To: <a4b26695-9388-0d69-ce81-3e4216846d22@tarantool.org> Hi, Roman! Sorry for the late response. Please, consider inline comments. WBR, Alexander Turenko. On Wed, Aug 15, 2018 at 01:29:38AM +0300, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! I have force pushed one minor > fix for alignment in lua_cjson.c. > > Alexander, please, make a second review. > > On 14/08/2018 02:14, roman.habibov1@yandex.ru wrote: > > - /* > > - * Update struct luaL_serializer using pointer to a > > - * configuration value (all values must be `int` for that). > > - */ Why this comment was stripped away? It seems to be still relevant to the new code. > > diff --git a/src/lua/utils.h b/src/lua/utils.h > > index 6b057af3e..9e2353511 100644 > > --- a/src/lua/utils.h > > +++ b/src/lua/utils.h > > @@ -240,6 +240,16 @@ luaL_checkserializer(struct lua_State *L) { > > luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER); > > } > > +/** > > + * Parse configuration table into @a cfg. Remove the lua table > > + * from the top of lua stack. It seems it does not remove the table from a stack. > > diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua > > index 42c79d6e9..ce21bbfcf 100755 > > --- a/test/app-tap/json.test.lua > > +++ b/test/app-tap/json.test.lua > > @@ -22,7 +22,53 @@ end > > tap.test("json", function(test) > > local serializer = require('json') > > - test:plan(13) > > + test:plan(25) > > + > > + -- gh-2888: Check the possibility of using options in encode()/decode(). > > + > > + local sub = {a = 1, { b = {c = 1, d = {e = 1}}}} Proposed to save encode_max_depth default value here, like so: local orig_encode_max_depth = serializer.cfg.encode_max_depth > > + 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') And restore it here: serializer.cfg({encode_max_depth = orig_encode_max_depth}) BTW, I think test case with {encode_max_depth = 2} is redundant. > > + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}', > > + 'depth of encoding is 1 with .encode') > > + Here we can check that json.encode(data, opts) did not change the global configuration: test:is(serializer.cfg.encode_max_depth, orig_encode_max_depth, 'global option remains unchanged') The same comments are applicable to other test cases. > > + local nan = 1/0 > > + test:ok(serializer.encode({a = nan}) == '{"a":inf}', > > + 'default "encode_invalid_numbers"') > > + serializer.cfg({encode_invalid_numbers = false}) > > + test:ok(not pcall(serializer.encode, {a = nan}), > > + 'expected error with NaN ecoding with .cfg') > > + serializer.cfg({encode_invalid_numbers = true}) > > + test:ok(not pcall(serializer.encode, {a = nan}, > > + {encode_invalid_numbers = false}), > > + 'expected error with NaN ecoding with .encode') > > + ecoding -> encoding (two occurences) > > + test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}', > > + {decode_max_depth = 2}), > > + 'error: too many nested data structures') > > + It is not obvious that gh-2888 block ends here. Please, add appropriate name for the block below or move gh-2888 block below the unnamed block. Look also how gh-3514 block header is formatted. It would be good to have all headers in the same style. > > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c > > index 11aa40225..e431c3fb0 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); > > - > > - lua_pushlstring(l, json, len); > > + if (lua_gettop(l) == 2) { > > + struct luaL_serializer user_cfg = *cfg; > > + luaL_serializer_parse_options(l, &user_cfg); > > + lua_pop(l, 1); lua_pop is used here (I guessto remove the table with options from a stack), but don't used in decode. > > + json_append_data(l, &user_cfg, 0, &encode_buf); > > + } else { > > + json_append_data(l, cfg, 0, &encode_buf); > > + } > > + char *json = strbuf_string(&encode_buf, NULL); > > + lua_pushlstring(l, json, encode_buf.length); encode_buf.length breaks incapsulation of strbuf 'object'. Please, use strbuf_length function instead. > > return 1; > > } > > @@ -977,9 +980,17 @@ static int json_decode(lua_State *l) > > json_token_t token; > > size_t json_len; > > - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); > > + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), > > + 1, "expected 1 or 2 arguments"); > > + > > + if (lua_gettop(l) == 2) { > > + struct luaL_serializer user_cfg = *luaL_checkserializer(l); > > + luaL_serializer_parse_options(l, &user_cfg); lua_pop is not used here, but used in encode. > > + json.cfg = &user_cfg; > > + } else { > > + json.cfg = luaL_checkserializer(l); > > + } > > - json.cfg = luaL_checkserializer(l); > > json.data = luaL_checklstring(l, 1, &json_len); > > json.current_depth = 0; > > json.ptr = json.data; > >
next prev parent reply other threads:[~2018-09-09 15:28 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1531010828.git.roman.habibov1@yandex.ru> 2018-07-08 0:57 ` [tarantool-patches] [PATCH] json: added " Roman Khabibov 2018-07-09 10:33 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-17 18:19 ` roman.habibov1 2018-07-19 10:18 ` Vladislav Shpilevoy 2018-07-23 22:38 ` [tarantool-patches] Re: [PATCH v3] json: add " roman.habibov1 2018-07-25 21:35 ` Vladislav Shpilevoy 2018-07-26 9:40 ` roman.habibov1 2018-07-26 10:07 ` Vladislav Shpilevoy 2018-07-26 12:29 ` roman.habibov1 2018-07-26 12:33 ` Vladislav Shpilevoy 2018-07-26 13:19 ` roman.habibov1 2018-07-26 21:45 ` Vladislav Shpilevoy 2018-07-31 15:29 ` roman.habibov1 2018-08-01 10:37 ` Vladislav Shpilevoy 2018-08-01 20:41 ` roman.habibov1 2018-08-02 12:59 ` Vladislav Shpilevoy 2018-08-07 21:52 ` roman.habibov1 2018-08-07 21:53 ` roman.habibov1 2018-08-08 19:07 ` Vladislav Shpilevoy 2018-08-13 23:14 ` roman.habibov1 2018-08-14 22:29 ` Vladislav Shpilevoy 2018-08-23 21:03 ` Alexander Turenko 2018-09-09 15:28 ` Alexander Turenko [this message] 2018-09-09 23:42 ` roman.habibov1 2018-09-10 13:12 ` Alexander Turenko 2018-08-08 19:08 ` Vladislav Shpilevoy 2018-07-11 7:57 ` [tarantool-patches] Re: [PATCH] json: added " Kirill Yukhin 2018-07-19 10:24 ` Vladislav Shpilevoy 2018-09-13 15:23 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180909152804.4lagt5drkymriv5x@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=roman.habibov1@yandex.ru \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox