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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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 "".

> diff --git a/src/box/info.h b/src/info.h
> similarity index 72%
> rename from src/box/info.h
> rename to src/info.h
> index df82becd5..bf1809ca9 100644
> --- a/src/box/info.h
> +++ b/src/info.h
> @@ -1,7 +1,7 @@
> -#ifndef INCLUDES_TARANTOOL_BOX_INFO_H
> -#define INCLUDES_TARANTOOL_BOX_INFO_H
> +#ifndef INCLUDES_TARANTOOL_INFO_H
> +#define INCLUDES_TARANTOOL_INFO_H
>  /*
> - * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
>   *
>   * Redistribution and use in source and binary forms, with or
>   * without modification, are permitted provided that the following
> @@ -35,9 +35,10 @@
>  
>  /**
>   * @file
> - * This module provides an adapter for Lua/C API to generate box.info()
> - * and index:info() introspection trees. The primary purpose of this
> - * adapter is to eliminate Engine <-> Lua interdependency.
> + * This module provides an adapter for Lua/C API to generate
> + * box.info() and index:info() introspection trees. The primary
> + * purpose of this adapter is to eliminate Engine <-> Lua
> + * interdependency.
>   *
>   * TREE STRUCTURE
>   *
> @@ -57,20 +58,18 @@
>   *
>   * IMPLEMENTATION DETAILS
>   *
> - * Current implementation calls Lua/C API under the hood without any
> - * pcall() wrapping. As you may now, idiosyncratic Lua/C API unwinds
> - * C stacks on errors in a way you can't handle in C. Please ensure that
> - * all blocks of code which call info_append_XXX() functions are
> - * exception/longjmp safe.
> + * Current implementation calls Lua/C API under the hood without
> + * any pcall() wrapping. As you may now, idiosyncratic Lua/C API
> + * unwinds C stacks on errors in a way you can't handle in C.
> + * Please ensure that all blocks of code which call
> + * info_append_XXX() functions are exception/longjmp safe.
>   */

Please refrain from gratuitous changes. The comment looks just fine as
it is. And even if there is a typo or the text width is a little longer
than recommended, there is no need to pollute the history to fix that -
every such change makes git-blame painful.

Please remove all uncalled for changes from this patch as well as other
patches of the series. Do only as much as you need to proceed.

>  
>  #if defined(__cplusplus)
>  extern "C" {
>  #endif /* defined(__cplusplus) */
>  
> -/**
> - * Virtual method table for struct info_handler.
> - */
> +/** Virtual method table for struct info_handler. */
>  struct info_handler_vtab {
>  	/** The begin of document. */
>  	void (*begin)(struct info_handler *);
> @@ -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.