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 7456A27452 for ; Thu, 19 Jul 2018 06:18:40 -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 nhJg4R4ySTYQ for ; Thu, 19 Jul 2018 06:18:40 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F017922E92 for ; Thu, 19 Jul 2018 06:18:39 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] json: added options to json.encode() References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> From: Vladislav Shpilevoy Message-ID: <12c20a86-8a40-4fe6-8dab-7b7b9494a142@tarantool.org> Date: Thu, 19 Jul 2018 13:18:37 +0300 MIME-Version: 1.0 In-Reply-To: <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: roman.habibov1@yandex.ru, "tarantool-patches@freelists.org" 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 > 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; > } >