From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E19F46EC55; Fri, 30 Jul 2021 11:44:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E19F46EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627634663; bh=Qf8oVqOlcsC/Xqads6teRy8TiCGTLKm7nvzNQZ1Au5k=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cajNC+x5REf1REyxRqFKrsAep0B+YrllU2bskqxaB86ctnrp17KyMtkCH3KFuEbYz jo/4MSjUeUV0NRA165Lu9wmS5B6YFrmucVhMJggAOC5ysjzo4SJGzPKUoN05XhzGKJ bUHL9IwPGpu0Q5kWOvAnLfhan8thHr7yyjwz0loI= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3AABB6EC55 for ; Fri, 30 Jul 2021 11:44:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3AABB6EC55 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1m9O7w-00044S-Dv; Fri, 30 Jul 2021 11:44:20 +0300 Date: Fri, 30 Jul 2021 11:44:19 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210730084419.vywinihhqdik74mr@esperanza> References: <6c3509125f24f6276be678d6b8f1ac264631d048.1627024646.git.vdavydov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD941C43E597735A9C30A5AB0699C09BB51E5FD76225F0C99C3182A05F538085040B3DBE97192C96B1F4C95155BD4C4EE0E4DC37C8212CE28FC05125E322ED0A815 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE798E808258C20928CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375D6F9E2FC7F7A8E98638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C67D59192C44E90E92D558607F4BE892117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3733B5EC72352B9FA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520B1593CA6EC85F86D618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6AC294AFEFA671E80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A583EA1380B9F1E34267507DC107CCB28607321203166BAFA6D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34060C3C6DE316ECE486DA8F8CDE89F19C9C16DE36F2DDF5ED413E470BFA05BD4A37A7F6AD93E8C1171D7E09C32AA3244C12D2E1D422E95392CACE11D604E534E560759606DA2E136AFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojWBddABnKmoJmOwjQdAVhag== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D6619A44437C071D066FBA4631C18FFC9274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 11/20] net.box: rewrite response decoder in C X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Fri, Jul 30, 2021 at 12:39:17AM +0200, Vladislav Shpilevoy wrote: > On 23.07.2021 13:07, Vladimir Davydov via Tarantool-patches wrote: > > +/** > > + * Decode Tarantool response body consisting of single > > + * IPROTO_DATA key into Lua table. > > + * @param L Lua stack to push result on. > > + * @param data MessagePack. > > + * @retval Lua table > > 1. It is not necessary to write these doxygen comments for each > param really if you think they are obvious. The only 'rule' is to > comment the function on its first declaration. Up to you of > course. Not a rule. You're right. Removed all the doxygen. > > +static void > > +netbox_decode_value(struct lua_State *L, const char **data, > > + const char *data_end, struct tuple_format *format) > > +{ > > + (void) data_end; > > + (void) format; > > + netbox_skip_to_data(data); > > + uint32_t count = mp_decode_array(data); > > + for (uint32_t i = 0; i < count; ++i) { > > + if (i == 0) > > + luamp_decode(L, cfg, data); > > + else > > + mp_next(data); > > 2. Is the compiler able to turn this into?: > > ... > if (count == 0) > return lua_pushnil(L); > > luamp_decode(L, cfg, data); > for (uint32_t i = 1; i < count; ++i) > mp_next(data); > } > > And why not do it right away? It looks shorter and a bit > simpler IMO. Up to you. Looks better, reworked, thanks. > > +static void > > +netbox_decode_tuple(struct lua_State *L, const char **data, > > + const char *data_end, struct tuple_format *format) > > +{ > > + (void) data_end; > > + netbox_skip_to_data(data); > > + uint32_t count = mp_decode_array(data); > > + for (uint32_t i = 0; i < count; ++i) { > > + const char *begin = *data; > > + mp_next(data); > > + if (i > 0) > > + continue; > > + struct tuple *tuple = box_tuple_new(format, begin, *data); > > + if (tuple == NULL) > > + luaT_error(L); > > + luaT_pushtuple(L, tuple); > > + } > > + if (count == 0) > > + lua_pushnil(L); > > 3. Ditto. Wouldn't it be shorter and a bit easier to read by doing this?: > > ... > if (count == 0) > return lua_pushnil(L); > > struct tuple *tuple = box_tuple_new(format, begin, *data); > if (tuple == NULL) > luaT_error(L); > luaT_pushtuple(L, tuple); > for (uint32_t i = 1; i < count; ++i) > mp_next(data); > } > > Up to you. Done. The incremental diff is below. The full patch is available by the link: https://github.com/tarantool/tarantool/commit/1087c1e687284efa4e39ce67fc4bea67265b7f58 -- diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 2d2562550752..ac9052de286c 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -664,7 +664,7 @@ netbox_encode_method(struct lua_State *L) return 0; } -/** +/* * This function handles a response that is supposed to have an empty body * (e.g. IPROTO_PING result). It doesn't decode anything per se. Instead it * simply pushes nil to Lua stack and advances the data ptr to data_end. @@ -678,7 +678,7 @@ netbox_decode_nil(struct lua_State *L, const char **data, lua_pushnil(L); } -/** +/* * This helper skips a MessagePack map header and IPROTO_DATA key so that * *data points to the actual response content. */ @@ -695,12 +695,9 @@ netbox_skip_to_data(const char **data) (void)key; } -/** - * Decode Tarantool response body consisting of single - * IPROTO_DATA key into Lua table. - * @param L Lua stack to push result on. - * @param data MessagePack. - * @retval Lua table +/* + * Decodes Tarantool response body consisting of single IPROTO_DATA key into + * a Lua table and pushes the table to Lua stack. */ static void netbox_decode_table(struct lua_State *L, const char **data, @@ -712,12 +709,9 @@ netbox_decode_table(struct lua_State *L, const char **data, luamp_decode(L, cfg, data); } -/** +/* * Same as netbox_decode_table, but only decodes the first element of the - * table, skipping the rest. Used to decode index.count() call result. - * @param L Lua stack to push result on. - * @param data MessagePack. - * @retval count or nil. + * table, skipping the rest. */ static void netbox_decode_value(struct lua_State *L, const char **data, @@ -727,20 +721,15 @@ netbox_decode_value(struct lua_State *L, const char **data, (void)format; netbox_skip_to_data(data); uint32_t count = mp_decode_array(data); - for (uint32_t i = 0; i < count; ++i) { - if (i == 0) - luamp_decode(L, cfg, data); - else - mp_next(data); - } if (count == 0) - lua_pushnil(L); + return lua_pushnil(L); + luamp_decode(L, cfg, data); + for (uint32_t i = 1; i < count; ++i) + mp_next(data); } -/** - * Decode IPROTO_DATA into tuples array. - * @param L Lua stack to push result on. - * @param data MessagePack. +/* + * Decodes IPROTO_DATA into a tuple array and pushes the array to Lua stack. */ static void netbox_decode_data(struct lua_State *L, const char **data, @@ -760,12 +749,9 @@ netbox_decode_data(struct lua_State *L, const char **data, } } -/** - * Decode Tarantool response body consisting of single - * IPROTO_DATA key into array of tuples. - * @param L Lua stack to push result on. - * @param data MessagePack. - * @retval Tuples array. +/* + * Decodes Tarantool response body consisting of single IPROTO_DATA key into + * tuple array and pushes the array to Lua stack. */ static void netbox_decode_select(struct lua_State *L, const char **data, @@ -776,12 +762,9 @@ netbox_decode_select(struct lua_State *L, const char **data, netbox_decode_data(L, data, format); } -/** +/* * Same as netbox_decode_select, but only decodes the first tuple of the array, * skipping the rest. - * @param L Lua stack to push result on. - * @param data MessagePack. - * @retval Tuple or nil. */ static void netbox_decode_tuple(struct lua_State *L, const char **data, @@ -790,18 +773,16 @@ netbox_decode_tuple(struct lua_State *L, const char **data, (void)data_end; netbox_skip_to_data(data); uint32_t count = mp_decode_array(data); - for (uint32_t i = 0; i < count; ++i) { - const char *begin = *data; - mp_next(data); - if (i > 0) - continue; - struct tuple *tuple = box_tuple_new(format, begin, *data); - if (tuple == NULL) - luaT_error(L); - luaT_pushtuple(L, tuple); - } if (count == 0) - lua_pushnil(L); + return lua_pushnil(L); + const char *begin = *data; + mp_next(data); + struct tuple *tuple = box_tuple_new(format, begin, *data); + if (tuple == NULL) + luaT_error(L); + luaT_pushtuple(L, tuple); + for (uint32_t i = 1; i < count; ++i) + mp_next(data); } /** Decode optional (i.e. may be present in response) metadata fields. */