[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

See 8 comments.

> commit ebef41bb68abbac75ed2d7cb4a5fa65d282f4ee3
> Author: Roman Khabibov <roman.habibov1 at yandex.ru>
> 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..12fa8ff0d 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -214,6 +214,50 @@ static struct {
>   	{ NULL, 0, 0, 0},
>   };
>   
> +/**
> + * Configure one field in @a cfg. Add one lua table to the top of
> + * lua stack.

1. Looks like the comment is deceptive. When the function returns
NULL, it pops the value and has no after effects. When it returns
not NULL, it pushes not table but integer. Please, rewrite the
comment more carefully.

2. This function as well as parse_options is method of luaL_serializer,
so it should has prefix luaL_serializer_ like luaL_serializer_cfg.

     luaL_serializer_parse_option
     luaL_serializer_parse_options

> + * @param L lua stack
> + * @param i index of option in OPTIONS[]
> + * @param cfg serializer to inherit configuration
> + * @return ponter to the value of option, NULL if option is not

3. Typo: 'ponter'.

4. For different retvals use separate @retval lines.

5. Use @retval instead of @return, as I said you in the
private chat.

> + * in the table
> + */
> +static int *
> +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); /* key hasn't changed */

6. Write comments on separate lines, start with capital letter,
finish with dot.

> +		return NULL;
> +	}

7. Lets always pop the value you got above before returning.
It is a strange behavior when a function sometimes pushes onto
the stack, and sometimes not. And I've found a bug caused by
this ambiguity. See the next comment.

> +	/*
> +	 * 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);
> +		break;
> +	case LUA_TNUMBER:
> +		*pval = lua_tointeger(L, -1);
> +		break;
> +	default:
> +		unreachable();
> +	}
> +	return pval;
> +}
> +
> +void
> +parse_options(struct lua_State *L, struct luaL_serializer *cfg) {
> +	for (int i = 0; OPTIONS[i].name != NULL; i++) {
> +		if (parse_option(L, i, cfg) != NULL)
> +			lua_pop(L, 1);
> +	}
> +	lua_pop(L, 1);
> +}
> +
>   /**
>    * @brief serializer.cfg{} Lua binding for serializers.
>    * serializer.cfg is a table that contains current configuration values from
> @@ -225,38 +269,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 */
>   	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);
> +		int *pval = parse_option(L, i, cfg);
>   		/* 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();
> +		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);
>   		}

8. Here you have a bug - the lua stack always grows. It was even
before your patch.

Before parse_option above you have stack size N. After parse_option
is called and returned not NULL you have stack size N + 1. After
lua_pushboolean/pushinteger you have stack size N + 2. After
lua_setfield it becomes N + 1. So on each iteration of the cycle
the stack grows by 1. It should not.

> -		/* Save normalized value to serializer.cfg table */
> -		lua_setfield(L, 1, OPTIONS[i].name);
>   	}
>   	return 0;
>   }