[tarantool-patches] Re: [PATCH 01/11] box: move info_handler interface into src/info

[Vladislav Shpilevoy]

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 <info.h>
> 
> 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.