Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: roman.habibov1@yandex.ru,
	"tarantool-patches@freelists.org"
	<tarantool-patches@freelists.org>
Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode()
Date: Thu, 26 Jul 2018 13:07:37 +0300	[thread overview]
Message-ID: <0e2c47c2-0276-a1da-c54b-726b64c17aae@tarantool.org> (raw)
In-Reply-To: <3367531532598057@sas1-4b7566131ec9.qloud-c.yandex.net>

Hi!

1. Build error.

[ 66%] Building C object src/CMakeFiles/server.dir/__/third_party/lua-cjson/lua_cjson.c.o
/Users/v.shpilevoy/Work/Repositories/tarantool/src/lua/utils.c:250:13: error: use of undeclared identifier 'l'
     lua_pop(l, 1);
             ^
1 error generated.


2. The patch you sent below differs from the one on the
branch. For example, first diff lines on the branch:

	diff --git a/src/lua/utils.c b/src/lua/utils.c
	index 2f0f4dcf8..012758b7f 100644
	--- a/src/lua/utils.c
	+++ b/src/lua/utils.c
	@@ -186,7 +186,6 @@ 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}
	 /**
	@@ -214,6 +213,44 @@ static struct {

But below I see another diff.

On 26/07/2018 12:40, roman.habibov1@yandex.ru wrote:
> Sorry. https://github.com/tarantool/tarantool/commit/768b875e498a17b3d5e404af79c1119b10a966e3
> 
> 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},
> +};
> +
> +int *
> +parse_option(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); /* key hasn't changed */
> @@ -223,6 +239,28 @@ int* parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {
>   	return pval;
>   }
>   
> +int
> +parse_options(lua_State *L, struct luaL_serializer *cfg) {
> +    for (int i = 0; OPTIONS[i].name != NULL; i++) {
> +        int *pval = parse_option(L, i, cfg);
> +        /* Update struct luaL_serializer structure */
> +        if (pval != NULL)
> +            lua_pop(L, 1);
> +    }
> +    lua_pop(l, 1);
> +    return 0;
> +}
> +
> +/**
> + * @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
> + */
>   static int
>   luaL_serializer_cfg(lua_State *L)
>   {
> @@ -244,8 +282,8 @@ luaL_serializer_cfg(lua_State *L)
>   			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/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"
>   /*
>    * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
>    *
> @@ -241,34 +240,29 @@ luaL_checkserializer(struct lua_State *L) {
>   		luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER);
>   }
>   
> -#define OPTION(type, name, defvalue) { #name, \
> -	offsetof(struct luaL_serializer, name), type, defvalue}
>   /**
> - * 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
>    */
> -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},
> -};
> +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);
>   
>   /** A single value on the Lua stack. */
>   struct luaL_field {
> 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()
>   
>       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}',
> -            '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}',
> -            'sub == {"1":{"b":null},"a":1}')
> +        'depth of encoding is 2 with .cfg')
>       serializer.cfg({encode_max_depth = 2})
> -    test:ok(serializer.encode(sub,{encode_max_depth = 1})
> -            == '{"1":null,"a":1}', 'sub == {"1":null,"a":1}')
> +    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}',
> -                              'a = nan == {"a":inf}')
> +        'default "encode_invalid_numbers"')
>       serializer.cfg({encode_invalid_numbers = false})
>       test:ok(pcall(serializer.encode, {a = nan}) == false,
> -            'error when "encode_invalid_numbers = false" with NaN')
> +        '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,
> -            'error when "encode_invalid_numbers = false" with 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')
> +        'precision more than 5')
>       serializer.cfg({encode_number_precision = 3})
>       test:ok(serializer.encode({a = number}) == '{"a":0.123}',
> -            'precision is 3')
> +        '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')
> +        {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')
> +
> +--
>       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 861079f8a..29553fc4d 100644
> --- a/third_party/lua-cjson/lua_cjson.c
> +++ b/third_party/lua-cjson/lua_cjson.c
> @@ -417,21 +417,9 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
>       }
>   }
>   
> -/*Serialize Lua table of options to luaL_serializer struct.*/
> -static int parse_options(lua_State *l, struct luaL_serializer* cfg) {
> -    for (int i = 0; OPTIONS[i].name != NULL; i++) {
> -        int *pval = parse_option(l, i, cfg);
> -        /* Update struct luaL_serializer structure */
> -        if (pval != NULL)
> -            lua_pop(l, 1);
> -    }
> -    lua_pop(l, 1);
> -    return 0;
> -}
> -
>   static int json_encode(lua_State *l) {
>       luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
> -                      1, "expected 1 or 2 arguments");
> +        1, "expected 1 or 2 arguments");
>   
>       /* Reuse existing buffer */
>       strbuf_reset(&encode_buf);
> @@ -443,7 +431,7 @@ static int json_encode(lua_State *l) {
>           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);
> @@ -993,7 +981,7 @@ static int json_decode(lua_State *l)
>       size_t json_len;
>   
>       luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
> -                      1, "expected 1 or 2 arguments");
> +        1, "expected 1 or 2 arguments");
>   
>       if (lua_gettop(l) == 2) {
>           struct luaL_serializer user_cfg = *luaL_checkserializer(l);
> 
> 26.07.2018, 00:35, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>> Hi! Thanks for the fixes!
>>
>> 1. Again, as I said on the previous review. Please, put a new
>> patch version at the end of letter.
>>
>> 2. On the branch I still see the old version. So looks like
>> you forgot to push. Please, do it and resend the patch.

  reply	other threads:[~2018-07-26 10:07 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 [this message]
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
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=0e2c47c2-0276-a1da-c54b-726b64c17aae@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=roman.habibov1@yandex.ru \
    --cc=tarantool-patches@freelists.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