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

[Vladislav Shpilevoy]

Hi! Thanks for working on the patch!

On 26/07/2018 16:19, roman.habibov1 at yandex.ru wrote:
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 2f0f4dcf8..3b89719ef 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);
>   }
>   
> -

1. Unnecessary diff. Please, avoid making additional
non-functional changes which increase number of hunks
in diff.

>   #define OPTION(type, name, defvalue) { #name, \
>   	offsetof(struct luaL_serializer, name), type, defvalue}
>   /**
> @@ -214,6 +213,44 @@ static struct {
>   	{ NULL, 0, 0, 0},
>   };
>   
> +int *
> +parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {

2. This function is not used out of utils.c and can be
declared as static and removed from the header.

3. Wrong style of luaL_serializer pointer declaration.
Please, put '*' next to the name and after whitespace
next to the type.

4. Please, do not omit 'struct' when declare struct
variables. Here I am pointing out lua_State parameter.

> +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 */

5. Below this line you do not update anything. What do you
mean?

6. Why do you need to announce pval? Why not just

     if (parse_option(L, i, cfg) != NULL)

?

> +        if (pval != NULL)
> +            lua_pop(L, 1);
> +    }
> +    lua_pop(L, 1);
> +    return 0;
> +}

7. Please, use 8-width tabs instead of spaces in this
file.

> +
>   /**
>    * @brief serializer.cfg{} Lua binding for serializers.
>    * serializer.cfg is a table that contains current configuration values from
> @@ -232,31 +269,22 @@ luaL_serializer_cfg(lua_State *L)
>   	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);

8. Please, fix pointers declaration style. The previous
was correct.

>   		/* 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);

9. Broken indentation.

> +				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);

10. Same.

> +		}
>   	}
>   	return 0;
>   }
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 6b057af3e..21eb36856 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -240,6 +240,30 @@ luaL_checkserializer(struct lua_State *L) {
>   		luaL_checkudata(L, lua_upvalueindex(1), 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.

11. Please, consult other places how to write function comments.
In a comment you should use imperative (like in the commit title)
and it is not mandatory thing to either duplicate function name
in the comment or write things like 'This function does blah-blah'.

Just write something like 'Parse configuration table into @a cfg ...'.

> + * @param L lua stack
> + * @param index of option in OPTIONS[]
> + * @param serializer to inherit configuration
> + * @return ponter to the value of option
> + */
> +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 {
>   	union {
> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
> index 3884b41e7..d76297ab5 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()

12. Please, finish the sentence with the dot. Yes, even
in the tests.