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 61DE82794E for ; Wed, 1 Aug 2018 06:37:45 -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 KjydFsbfZCas for ; Wed, 1 Aug 2018 06:37:45 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 9D36F25063 for ; Wed, 1 Aug 2018 06:37:44 -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> <33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org> <28658371533050946@myt4-74a8acfc13eb.qloud-c.yandex.net> From: Vladislav Shpilevoy Message-ID: Date: Wed, 1 Aug 2018 13:37:42 +0300 MIME-Version: 1.0 In-Reply-To: <28658371533050946@myt4-74a8acfc13eb.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! See 8 comments. > commit ebef41bb68abbac75ed2d7cb4a5fa65d282f4ee3 > 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..12fa8ff0d 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -214,6 +214,50 @@ static struct { > { NULL, 0, 0, 0}, > }; > > +/** > + * Configure one field in @a cfg. Add one lua table to the top of > + * lua stack. 1. Looks like the comment is deceptive. When the function returns NULL, it pops the value and has no after effects. When it returns not NULL, it pushes not table but integer. Please, rewrite the comment more carefully. 2. This function as well as parse_options is method of luaL_serializer, so it should has prefix luaL_serializer_ like luaL_serializer_cfg. luaL_serializer_parse_option luaL_serializer_parse_options > + * @param L lua stack > + * @param i index of option in OPTIONS[] > + * @param cfg serializer to inherit configuration > + * @return ponter to the value of option, NULL if option is not 3. Typo: 'ponter'. 4. For different retvals use separate @retval lines. 5. Use @retval instead of @return, as I said you in the private chat. > + * in the table > + */ > +static int * > +parse_option(struct 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 */ 6. Write comments on separate lines, start with capital letter, finish with dot. > + return NULL; > + } 7. Lets always pop the value you got above before returning. It is a strange behavior when a function sometimes pushes onto the stack, and sometimes not. And I've found a bug caused by this ambiguity. See the next comment. > + /* > + * 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; > +} > + > +void > +parse_options(struct lua_State *L, struct luaL_serializer *cfg) { > + for (int i = 0; OPTIONS[i].name != NULL; i++) { > + if (parse_option(L, i, cfg) != NULL) > + lua_pop(L, 1); > + } > + lua_pop(L, 1); > +} > + > /** > * @brief serializer.cfg{} Lua binding for serializers. > * serializer.cfg is a table that contains current configuration values from > @@ -225,38 +269,29 @@ static struct { > * @return 0 > */ > static int > -luaL_serializer_cfg(lua_State *L) > +luaL_serializer_cfg(struct lua_State *L) > { > luaL_checktype(L, 1, LUA_TTABLE); /* serializer */ > luaL_checktype(L, 2, LUA_TTABLE); /* serializer.cfg */ > 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); > } 8. Here you have a bug - the lua stack always grows. It was even before your patch. Before parse_option above you have stack size N. After parse_option is called and returned not NULL you have stack size N + 1. After lua_pushboolean/pushinteger you have stack size N + 2. After lua_setfield it becomes N + 1. So on each iteration of the cycle the stack grows by 1. It should not. > - /* Save normalized value to serializer.cfg table */ > - lua_setfield(L, 1, OPTIONS[i].name); > } > return 0; > }