Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Nikita Pettik <korablev@tarantool.org>]

On 14 May 00:10, Vladislav Shpilevoy wrote:
> Thanks for the review!
> 
> On 13/05/2020 14:31, Nikita Pettik wrote:
> > On 12 May 01:45, Vladislav Shpilevoy wrote:
> >> +#define MP_ERROR_PRINT_DEFINITION
> >> +#define MP_PRINT_FUNC fprintf
> >> +#define MP_PRINT_SUFFIX fprint
> >> +#define MP_PRINT_2(total, func, ...) do {							\
> >> +	int bytes = func(file, __VA_ARGS__);					\
> >> +	if (bytes < 0)								\
> >> +		return -1;							\
> >> +	total += bytes;								\
> >> +} while (0)
> >> +#define MP_PRINT_ARGS_DECL FILE *file
> >> +#include __FILE__
> > 
> > I've failed to understand how this is supposed to work.
> > Why do you have to include file itself? Why do you have to do it twice?
> > This macro related part definitely lacks some comments.
> 
> This is not supposed to work. It works already. Was a surprise even for me.
> I wasn't sure this needs more explanation that I gave below:
> 
> > /**
> >  * MP_ERROR extension string serializer.
> >  * There are two applications for string serialization - into a
> >  * buffer, and into a file. Structure of both is exactly the same
> >  * except for the copying/writing itself. To avoid code
> >  * duplication the code is templated and expects some macros to do
> >  * the actual output.
> >  */
> This is basically a C template. The same as bps tree, rb tree, mhash. But
> located in the same source file which needs it. Since this is not a common
> enough feature to move it to a separate place.
> 
> I include self with pre-declared macros MP_ERROR_PRINT_DEFINITION. When it
> is defined, the file skips everything except the 3 functions in the end:
> mp_print_error_one(), mp_print_error_stack(), mp_print_error().

A-ha, it is what I missed. But personally I would anyway move these
macros/functions to a separate file. It would definitely cut extra
questions and would make code cleaner (IMHO) (I bet anybody who
sees '#include __FILE__' for the first time is like 'WTF?!').
 
> The skip uses mere #ifdef MP_ERROR_PRINT_DEFINITION to understand, that
> currently the file is being included into self to customize the error
> printers. So all is skipped except them.
> 
> I include the file twice, but with different parameters. Like if I would
> customize one template with two parameter sets in C++. First inclusion
> turns
> 
>     mp_print_error_one(), mp_print_error_stack(), mp_print_error()
> 
> into
> 
>     mp_snprint_error_one(), mp_snprint_error_stack(), mp_snprint_error()
> 
> and snprintf() calls inside. The second inclusion turns the functions into
> 
>     mp_fprint_error_one(), mp_fprint_error_stack(), mp_fprint_error()
> 
> and fprintf() calls inside.
> 
> I tried to extend the comment.

You gave cool explanation right in this message. Why not copy it to
the source file?

> ====================
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 0b5e6bc96..a4a4ccaca 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -587,6 +587,21 @@ error_unpack_unsafe(const char **data)
>   * except for the copying/writing itself. To avoid code
>   * duplication the code is templated and expects some macros to do
>   * the actual output.
> + *
> + * Templates are defined similarly to the usual C trick, when all
> + * the code is in one file and relies on some external macros to
> + * define customizable types, functions, parameters. Then other
> + * code can include this file with the macros defined. Even
> + * multiple times, in case structure and function names also are
> + * customizable.
> + *
> + * The tricky thing here is that the templates are defined in the
> + * same file, which needs them. So the file needs to include
> + * *self* with a few macros defined beforehand. That allows to
> + * hide this template from any external access, use mp_error
> + * internal functions, and keep all the code in one place without
> + * necessity to split it into different files just to be able to
> + * turn it into a template.
>   */
>