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 D029927AAE for ; Tue, 7 Aug 2018 17:53:04 -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 nWUb6KtsZUZl for ; Tue, 7 Aug 2018 17:53:04 -0400 (EDT) Received: from forward3j.cmail.yandex.net (forward3j.cmail.yandex.net [5.255.227.21]) (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 6B9A5278F1 for ; Tue, 7 Aug 2018 17:53:04 -0400 (EDT) From: roman.habibov1@yandex.ru In-Reply-To: <26831531533678724@myt5-f9d71769b752.qloud-c.yandex.net> 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> <26831531533678724@myt5-f9d71769b752.qloud-c.yandex.net> Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() MIME-Version: 1.0 Date: Wed, 08 Aug 2018 00:53:01 +0300 Message-Id: <27864721533678781@sas1-38f90f4f25ea.qloud-c.yandex.net> Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" 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" Addition. New branch: https://github.com/tarantool/tarantool/tree/romankhabibov/gh-2888-options-encode 08.08.2018, 00:52, "roman.habibov1@yandex.ru" : > Hi! Thanks for the review! > >>  1. Put the branch onto 1.10, not on 2.0. It does not >>  require SQL, so 1.10 is ok. > > Done. > >>  2. As I said earlier, please, use separate @retval for each >>  possible value. One @retval for not NULL pointer, one @retval >>  for NULL. > > +/** > + * 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 > + * @retval NULL if option is not > + * in the table > + */ > +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. > > +luaL_serializer_parse_option(struct lua_State *L, int i, > + struct luaL_serializer *cfg) > > +void > +luaL_serializer_parse_options(struct lua_State *L, > + struct luaL_serializer *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. > > 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 */ > - 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(); > - } > - /* Save normalized value to serializer.cfg table */ > - lua_setfield(L, 1, OPTIONS[i].name); > - } > + /* Iterate over all available options and checks keys in passed table. */ > + for (int i = 0; OPTIONS[i].name != NULL; i++) > + luaL_serializer_parse_option(L, i, cfg); >          return 0; >  } > >>  5. Please, align function's lines by the function's body offset. Including >>  comments. > > Fixed. > >>  6. You've called the same cfg 2 lines above. > > + 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') > + 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. > > Fixed. > >>  8. I believe, instead of 'boolean == false' you can just do >>  'not boolean' (I've fixed it here. Do the same in other >>  places). > > + test:ok(not pcall(serializer.encode, {a = nan}), > + 'expected error with NaN ecoding with .cfg') > + test:ok(not pcall(serializer.encode, {a = nan}, > + {encode_invalid_numbers = false}), > + 'expected error with NaN ecoding with .encode') > > + test:ok(not pcall(serializer.decode, '{"a":inf}', > + {decode_invalid_numbers = false}), > + 'expected error with NaN decoding with .decode') > >>  9. Garbage empty comment. > > Deleted. > >>  10. 'len' parameter is ok to be NULL here. > > + char *json = strbuf_string(&encode_buf, NULL); > + lua_pushlstring(l, json, encode_buf.length); > > commit f6d41bdfcec241733f98b7d26715ece6625539aa > 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..3faeb416f 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -214,6 +214,48 @@ 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 > + * @retval NULL if option is not > + * in the table > + */ > +static int * > +luaL_serializer_parse_option(struct lua_State *L, int i, > + struct luaL_serializer *cfg) > +{ > + 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,39 +267,14 @@ 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 */ > - 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(); > - } > - /* Save normalized value to serializer.cfg table */ > - lua_setfield(L, 1, OPTIONS[i].name); > - } > + /* Iterate over all available options and checks keys in passed table. */ > + for (int i = 0; OPTIONS[i].name != NULL; i++) > + luaL_serializer_parse_option(L, i, cfg); >          return 0; >  } > > diff --git a/src/lua/utils.h b/src/lua/utils.h > index 6b057af3e..204bd8664 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. > + * @param L lua stack > + * @param cfg serializer to inherit configuration > + */ > +void > +luaL_serializer_parse_options(struct lua_State *l, > + struct luaL_serializer *cfg); > + >  /** A single value on the Lua stack. */ >  struct luaL_field { >          union { > diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua > index 42c79d6e9..f2c67ab0a 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(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}}}} > + 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') > + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}', > + 'depth of encoding is 1 with .encode') > + > + 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') > + > + 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(not pcall(serializer.decode, '{"a":inf}'), > + 'expected error with NaN decoding with .cfg') > + serializer.cfg({decode_invalid_numbers = true}) > + test:ok(not pcall(serializer.decode, '{"a":inf}', > + {decode_invalid_numbers = false}), > + 'expected error with NaN decoding with .decode') > + > + test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}', > + {decode_max_depth = 2}), > + 'error: too many nested data structures') > + >      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 11aa40225..b175b770c 100644 > --- a/third_party/lua-cjson/lua_cjson.c > +++ b/third_party/lua-cjson/lua_cjson.c > @@ -417,22 +417,24 @@ 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); > + 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); >      return 1; >  } > > @@ -977,9 +979,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); > + 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;