From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 01/11] box: move info_handler interface into src/info References: <265787a088a3b0625966ee726c16831e5cc877e4.1543590433.git.v.shpilevoy@tarantool.org> <20181203110559.a4zpgajkuh7sw2ir@esperanza> From: Vladislav Shpilevoy Message-ID: <55bbc588-243d-7379-c4b4-3d61f90a422b@tarantool.org> Date: Tue, 4 Dec 2018 00:48:23 +0300 MIME-Version: 1.0 In-Reply-To: <20181203110559.a4zpgajkuh7sw2ir@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: Hi! Thanks for the review! On 03/12/2018 14:05, Vladimir Davydov wrote: > On Fri, Nov 30, 2018 at 06:39:31PM +0300, Vladislav Shpilevoy wrote: >> Box/info.h defines info_handler interface with a set >> of virtual functions. It allows to hide Lua from code >> not depending on this language, and is used in things >> like index:info(), box.info() to build Lua table with >> some info. But it does not depend on box/ so move it >> to src/. And alongside apply a bit refactoring: remove >> useless comments, comply with line width etc. >> >> Also, this API is needed for the forthcoming SWIM >> module which is going to be placed into src/. >> >> Needed for #3234 >> --- >> src/CMakeLists.txt | 1 + >> src/box/lua/index.c | 4 +- >> src/box/lua/info.c | 78 +--------------------------- >> src/box/lua/sql.c | 4 +- >> src/box/lua/stat.c | 4 +- >> src/box/sql.c | 2 +- >> src/{box => }/info.h | 84 +++++++++++++++--------------- >> src/lua/info.c | 118 +++++++++++++++++++++++++++++++++++++++++++ >> src/lua/info.h | 49 ++++++++++++++++++ >> 9 files changed, 218 insertions(+), 126 deletions(-) >> rename src/{box => }/info.h (72%) >> create mode 100644 src/lua/info.c >> create mode 100644 src/lua/info.h > > This patch has nothing to do with the rest of the series, which is > devoted to removing exceptions from evio, so it can be reviewed and > committed independently. Please submit the next version of this patch > separately. > >> diff --git a/src/box/lua/index.c b/src/box/lua/index.c >> index ef89c397d..6265c044a 100644 >> --- a/src/box/lua/index.c >> +++ b/src/box/lua/index.c >> @@ -30,10 +30,10 @@ >> */ >> #include "box/lua/index.h" >> #include "lua/utils.h" >> +#include "lua/info.h" >> +#include > > We typically use angular brackets <> only for system headers and > external libraries. For the rest of the source code, including src/, > we prefer quotes "". Unfortunately, now it is the only way to resolve duplicate headers like this: src/info.h src/lua/info.h and src/fiber.h src/lua/fiber.h and src/fiber_channel.h src/lua/fiber_channel.h and src/fiber_cond.h src/lua/fiber_cond.h and src/fio.h src/lua/fio.h and src/trigger.h src/lua/trigger.h etc Also we use <> for many small/ header appearances. So yes, you are right, we prefer, but "" is not mandatory. We use <> when we are in src/lua/ and want to include header src/#some_file# instead of src/lua/#some_file#. "" includes local header, but we need global. Many ways exist how to solve this problem but none of them shall be part of this patch. I remind that swim is going to be a separate module is src/lib and it needs info to dump swim.info() from C to Lua without strict dependency on Lua. >> @@ -86,13 +85,17 @@ struct info_handler_vtab { >> /** Set int64_t value. */ >> void (*append_int)(struct info_handler *, const char *key, >> int64_t value); >> + /** Set uint64_t value. */ >> + void (*append_uint)(struct info_handler *, const char *key, >> + uint64_t value); > > Why would one need append_uint() when one can pass uint64_t to > append_int(). I deliberately removed append_uint() a while back. > I don't think there's any need to reintroduce this function. > I need it since SWIM has unsigned incarnation number adhered to each cluster member. I want to show it in swim.info() at least for tests so I returned append_uint back. Talking about passing uint to append_int - I do not want to cast. Also, API will not be symmetric as you usually require in your own reviews, but as you wish.