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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the review!

On 13/05/2020 14:31, Nikita Pettik wrote:
> On 12 May 01:45, Vladislav Shpilevoy wrote:
>>  src/box/mp_error.h        |  29 ++++
>>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>>  test/unit/mp_error.result |  72 +++++++++-
>>  4 files changed, 529 insertions(+), 3 deletions(-)
>> +
>> +#define MP_ERROR_PRINT_DEFINITION
>> +#define MP_PRINT_FUNC snprintf
>> +#define MP_PRINT_SUFFIX snprint
>> +#define MP_PRINT_2(total, func, ...)						\
>> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
>> +#define MP_PRINT_ARGS_DECL char *buf, int size
>> +#include __FILE__
>> +
>> +#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().

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.

====================
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.
  */