From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: roman.habibov1@yandex.ru, "tarantool-patches@freelists.org" <tarantool-patches@freelists.org> Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() Date: Fri, 27 Jul 2018 00:45:05 +0300 [thread overview] Message-ID: <33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org> (raw) In-Reply-To: <2952461532611162@iva5-08e20335b0d9.qloud-c.yandex.net> 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.
next prev parent reply other threads:[~2018-07-26 21:45 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1531010828.git.roman.habibov1@yandex.ru> 2018-07-08 0:57 ` [tarantool-patches] [PATCH] json: added " Roman Khabibov 2018-07-09 10:33 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-17 18:19 ` roman.habibov1 2018-07-19 10:18 ` Vladislav Shpilevoy 2018-07-23 22:38 ` [tarantool-patches] Re: [PATCH v3] json: add " roman.habibov1 2018-07-25 21:35 ` Vladislav Shpilevoy 2018-07-26 9:40 ` roman.habibov1 2018-07-26 10:07 ` Vladislav Shpilevoy 2018-07-26 12:29 ` roman.habibov1 2018-07-26 12:33 ` Vladislav Shpilevoy 2018-07-26 13:19 ` roman.habibov1 2018-07-26 21:45 ` Vladislav Shpilevoy [this message] 2018-07-31 15:29 ` roman.habibov1 2018-08-01 10:37 ` Vladislav Shpilevoy 2018-08-01 20:41 ` roman.habibov1 2018-08-02 12:59 ` Vladislav Shpilevoy 2018-08-07 21:52 ` roman.habibov1 2018-08-07 21:53 ` roman.habibov1 2018-08-08 19:07 ` Vladislav Shpilevoy 2018-08-13 23:14 ` roman.habibov1 2018-08-14 22:29 ` Vladislav Shpilevoy 2018-08-23 21:03 ` Alexander Turenko 2018-09-09 15:28 ` Alexander Turenko 2018-09-09 23:42 ` roman.habibov1 2018-09-10 13:12 ` Alexander Turenko 2018-08-08 19:08 ` Vladislav Shpilevoy 2018-07-11 7:57 ` [tarantool-patches] Re: [PATCH] json: added " Kirill Yukhin 2018-07-19 10:24 ` Vladislav Shpilevoy 2018-09-13 15:23 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=roman.habibov1@yandex.ru \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox