[tarantool-patches] Re: [PATCH] json: added options to json.encode()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Jul 19 13:18:37 MSK 2018
Hi! Thanks for the fixes! Please, put a new version of
the patch at the end of email sending the former. Now I
did it for you.
See 10 comments below.
> commit a229af626a78c7f37b628101e189f7727c01ccd8
> 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..0626bf76b 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -186,34 +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}
1. You should not touch this hunk. Utils should provide
only API to configure the serializer, and do not expose
options array.
> -/**
> - * Configuration options for serializers
> - * @sa struct luaL_serializer
> - */
> -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},
> -};
> -
> /**
> * @brief serializer.cfg{} Lua binding for serializers.
> * serializer.cfg is a table that contains current configuration values from
> @@ -224,6 +196,33 @@ static struct {
> * @param L lua stack
> * @return 0
> */
2. You've cut the comment off. It belongs to another function.
> +
> +/*Serialize one option. Returns ponter to the value of option.*/
> +int* parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {
3. Bad style. Please, see in other functions how to write comments
and how to declare pointers. You should not write int*, but int *.
Please, wrap line after the function return type declaring.
> + lua_getfield(L, 2, OPTIONS[i].name);
> + if (lua_isnil(L, -1)) {
> + lua_pop(L, 1); /* key hasn't changed */
> + return NULL;
> + }
> + /*
> + * 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;
> +}
> +
> static int
> luaL_serializer_cfg(lua_State *L)
> {
> @@ -232,31 +231,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);
> /* 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);
4. 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);
> }
> - /* 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 6b057af3e..4a2c3eaac 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -1,5 +1,6 @@
> #ifndef TARANTOOL_LUA_UTILS_H_INCLUDED
> #define TARANTOOL_LUA_UTILS_H_INCLUDED
> +#pragma GCC diagnostic ignored "-Wunused-variable"
5. Garbage diff. You should never change compilation
options via pragma. And if you need this one, then your
patch has a problem, that should be fixed, not hidden.
> /*
> * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
> *
> @@ -240,6 +241,35 @@ 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
> + */
> +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);
6. Please, write function comment on top of the declaration. Not on
top of the implementation.
> +
> /** 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..050d769ea 100755
> --- a/test/app-tap/json.test.lua
> +++ b/test/app-tap/json.test.lua
> @@ -22,7 +22,42 @@ end
>
> tap.test("json", function(test)
> local serializer = require('json')
> - test:plan(9)
> + test:plan(18)
> +
> +-- gh-2888 Added opions to encode().
7. Typo: opions.
8. Please, consult other test files how to write a test comment.
It should not be a description of what was done, but an explanation
of the problem.
9. I do not see tests on decode.
> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
> index aa8217dfb..861079f8a 100644
> --- a/third_party/lua-cjson/lua_cjson.c
> +++ b/third_party/lua-cjson/lua_cjson.c
> @@ -417,22 +417,37 @@ 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;
> +/*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;
> +}
>
> - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument");
> +static int json_encode(lua_State *l) {
> + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
> + 1, "expected 1 or 2 arguments");
10. Bad indentation. In decode too.
>
> /* 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);
> + if (lua_gettop(l) == 2) {
> + struct luaL_serializer user_cfg = *cfg;
> + parse_options(l, &user_cfg);
> + 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);
> lua_pushlstring(l, json, len);
> -
> return 1;
> }
>
More information about the Tarantool-patches
mailing list