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 F14552717A for ; Thu, 26 Jul 2018 17:45:07 -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 exKBWhjCnRcK for ; Thu, 26 Jul 2018 17:45:07 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 AF770216EF for ; Thu, 26 Jul 2018 17:45:07 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> <8327181531851586@myt4-ec1fcebe7be6.qloud-c.yandex.net> <12c20a86-8a40-4fe6-8dab-7b7b9494a142@tarantool.org> <39430371532385504@iva8-37fc2ad204cd.qloud-c.yandex.net> <3367531532598057@sas1-4b7566131ec9.qloud-c.yandex.net> <0e2c47c2-0276-a1da-c54b-726b64c17aae@tarantool.org> <5209131532608150@myt3-c7e5d17fe013.qloud-c.yandex.net> <2952461532611162@iva5-08e20335b0d9.qloud-c.yandex.net> From: Vladislav Shpilevoy Message-ID: <33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org> Date: Fri, 27 Jul 2018 00:45:05 +0300 MIME-Version: 1.0 In-Reply-To: <2952461532611162@iva5-08e20335b0d9.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 working on the patch! On 26/07/2018 16:19, roman.habibov1@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.