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 5BD4326597 for ; Mon, 9 Jul 2018 06:33:36 -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 G-uHvU40QOCu for ; Mon, 9 Jul 2018 06:33:36 -0400 (EDT) Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95]) (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 1782B22291 for ; Mon, 9 Jul 2018 06:33:35 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] json: added options to json.encode() References: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> From: Vladislav Shpilevoy Message-ID: Date: Mon, 9 Jul 2018 13:33:33 +0300 MIME-Version: 1.0 In-Reply-To: <693ca18f728ee462e28861841004ff77bf62bfb5.1531010828.git.roman.habibov1@yandex.ru> 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: tarantool-patches@freelists.org, Roman Khabibov Hello. Thanks for the patch! My congratulations on your second patch! See and fix 10 comments below to make your patch better. 1. Please, use imperative in commit header. Not 'json: added options to json.encode()', but 'json: add options to json.encode()'. On 08/07/2018 03:57, Roman Khabibov wrote: > Added an ability to pass encoder options to json.encode(). > > Closes: #2888. > --- > test/app-tap/json.test.lua | 27 +++++++++++- > third_party/lua-cjson/lua_cjson.c | 90 +++++++++++++++++++++++++++++++++------ > 2 files changed, 104 insertions(+), 13 deletions(-) > > diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua > index 3884b41..1252ad0 100755 > --- a/test/app-tap/json.test.lua > +++ b/test/app-tap/json.test.lua 2. I will review tests later when we will finish with the other code. > 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(). > +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. > > - 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. > > - 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 }. 8. Put the dot at the end of sentence. > + 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; > } 10. Please, add the options to json.decode. API must be symmetrical. > > /* ===== DECODING ===== */ >