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 C813A277AD for ; Tue, 17 Jul 2018 14:19:49 -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 W_1w6BRKxWR0 for ; Tue, 17 Jul 2018 14:19:49 -0400 (EDT) Received: from forward5p.cmail.yandex.net (forward5p.cmail.yandex.net [77.88.31.20]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 075E1277AC for ; Tue, 17 Jul 2018 14:19:48 -0400 (EDT) From: roman.habibov1@yandex.ru In-Reply-To: References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> Subject: [tarantool-patches] Re: [PATCH] json: added options to json.encode() MIME-Version: 1.0 Date: Tue, 17 Jul 2018 21:19:46 +0300 Message-Id: <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" 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: Vladislav Shpilevoy , "tarantool-patches@freelists.org" > 1. Please, use imperative in commit header. Fixed. >>  diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c >>  index aa8217d..bdfda1e 100644 >>  --- a/third_party/lua-cjson/lua_cjson.c >>  +++ b/third_party/lua-cjson/lua_cjson.c >>  @@ -417,23 +417,89 @@ 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; >>  +#define OPTION(type, name, defvalue) { #name, \ >>  + offsetof(struct luaL_serializer, name), type, defvalue} > > 3. All this code including parse_options is a full copy of > the code from src/lua/util.c. Please, find a way how to do not > duplicate the code. You should consolidate an options parser > function from util.c code and make a declaration in a header. > And then use this function both in util.c in luaL_serializer_cfg() > and in json_encode(). > 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} -/** - * 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 */ + +/*Serialize one option. Returns ponter to the value of option.*/ +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 */ + 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); + 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" /* * 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); + /** A single value on the Lua stack. */ struct luaL_field { union { diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c index bdfda1e79..861079f8a 100644 --- a/third_party/lua-cjson/lua_cjson.c +++ b/third_party/lua-cjson/lua_cjson.c @@ -417,89 +417,38 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg, } } -#define OPTION(type, name, defvalue) { #name, \ - offsetof(struct luaL_serializer, name), type, defvalue} - -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}, -}; - -/*Serialize Lua table of options to luaL_serializer struct*/ +/*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++) { - 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); - /* Update struct luaL_serializer structure */ - switch (OPTIONS[i].type) { - case LUA_TBOOLEAN: - *pval = lua_toboolean(l, -1); - lua_pop(l, 1); - break; - case LUA_TNUMBER: - *pval = lua_tointeger(l, -1); - lua_pop(l, 1); - break; - default: - unreachable(); - } - } - lua_pop(l, 1); - return 0; + 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"); > > 4. Out of 80 symbols. > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c index bdfda1e79..861079f8a 100644 - luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), 1, "expected 1 or 2 arguments"); + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), + 1, "expected 1 or 2 arguments"); >>  - json_append_data(l, cfg, 0, &encode_buf); >>  - json = strbuf_string(&encode_buf, &len); >>  + char *json; >>  + int len; > > 5. In the original code spaces were used, not tabs. Please, > use spaces in this file too. > > 6. You do not need to annotate those arguments here. Please, > declare them together with assigning. > > Not > var name; > name = value; > > But > var name = value. > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c index bdfda1e79..861079f8a 100644 - char *json; - int len; + int len; + char *json = strbuf_string(&encode_buf, &len); >>  - lua_pushlstring(l, json, len); >>  + /* Reuse existing buffer */ >>  + strbuf_reset(&encode_buf); >>  + struct luaL_serializer *cfg = luaL_checkserializer(l); >> >>  - return 1; >>  + /* If have second arg */ >>  + 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); >>  + } >>  + /* If have not second arg */ >>  + else { > > 7. Please, put 'else' on the same line as }. Fixed. > 8. Put the dot at the end of sentence. > >>  + Fixed. > 9. Extra empty line. > >>  + json_append_data(l, cfg, 0, &encode_buf); >>  + } >>  + >>  + json = strbuf_string(&encode_buf, &len); >>  + lua_pushlstring(l, json, len); >>  + >>  + return 1; >>    } Fixed. > 10. Please, add the options to json.decode. API must be symmetrical. diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c index bdfda1e79..861079f8a 100644 /* ===== DECODING ===== */ @@ -1043,9 +992,17 @@ static int json_decode(lua_State *l) json_token_t token; size_t json_len; - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), + 1, "expected 1 or 2 arguments"); + + if (lua_gettop(l) == 2) { + struct luaL_serializer user_cfg = *luaL_checkserializer(l); + parse_options(l, &user_cfg); + json.cfg = &user_cfg; + } else { + json.cfg = luaL_checkserializer(l); + } - json.cfg = luaL_checkserializer(l); json.data = luaL_checklstring(l, 1, &json_len); json.current_depth = 0; json.ptr = json.data;