From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1270F469710 for ; Mon, 18 May 2020 18:24:28 +0300 (MSK) References: <22a830fb9d2dbc3883b9710d36ab88c638101002.1589240704.git.v.shpilevoy@tarantool.org> <20200512175205.GF2219@grain> <6c8dc667-43ce-997b-1b1f-cf725f05a2f8@tarantool.org> From: Serge Petrenko Message-ID: <04b2db4d-20ea-4055-2518-d66b65aefdd8@tarantool.org> Date: Mon, 18 May 2020 18:24:27 +0300 MIME-Version: 1.0 In-Reply-To: <6c8dc667-43ce-997b-1b1f-cf725f05a2f8@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! 12.05.2020 23:38, Vladislav Shpilevoy пишет: > Thanks for the review! > > On 12/05/2020 19:52, Cyrill Gorcunov wrote: >> On Tue, May 12, 2020 at 01:45:51AM +0200, Vladislav Shpilevoy wrote: >>> Msgpuck functions mp_snprint() and mp_fprint() now support >>> customizable MP_EXT serializer. This patch adds such for MP_ERROR. >>> All extension serializers will be activated in a separate commit. >>> >>> Part of #4719 >>> --- >>> src/box/mp_error.cc | 161 ++++++++++++++++++++++- >>> 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(-) >>> >>> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc >>> index 0491a7a50..fed2ce288 100644 >>> --- a/src/box/mp_error.cc >>> +++ b/src/box/mp_error.cc >>> @@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data) >>> } >>> return err; >>> } >>> + >>> +#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__ >> Don't get it -- include current file?! This a good sign of >> some kind of problem in the code structure... If we need >> a template then it should be some .cc/.c file explicily >> included instead of __FILE__, no? > I don't want to introduce a new file just for these 2 functions > (mp_fprint_error() and mp_snprint_error()). This would be even > worse than it is now. So I used the same file. I wrote __FILE__ > instead of mp_error.cc, so as to minimize diff, when mp_error.cc > will be converted to C and moved to lib/core (that is planned). > Also #include __FILE__ is more clear, when you want to say > "include self", IMO. > > Alternative is to make mp_print_error_one(), mp_print_error_stack(), > mp_print_error() as huge macros, but I don't like writing big macros. > It is hard to edit, and much much harder to debug, since this all > becomes just one infinite line, and 'n' in gdb/lldb skips the whole > function. > > If you don't like the whole template idea, there is one alternative > I described in the cover letter for the msgpuck patchset - virtual > printer. But it will be really slow. We could probably try to make it > buffered, but then it will be double-copying, also slow. I tried really > hard to find other solutions, but failed. Even looked at fmemopen(), > so as we would need to implement only mp_fprint(), but msgpuck library > is supposed to work on Windows too. Also fmemopen() is basically a > virtual printer as well (i.e. is slower, than direct snprint()). > > So if you have anything in mind except templates, or a virtual printer, > or you know something about fmemopen() what I don't know, please, tell. > Because I don't like these solutions, but just couldn't find anything > better, with bearable complexity worth the feature. Which I thought would > be just one-day patch, but I was very very wrong here. > > I could switch to the virtual printer in case you think its slowness is > worth the simplification. > > As unbearable level of complexity I mean the non-binary tree solution > I proposed in the msgpuck patchset's cover letter. > >>> +#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__ >>> + >>> +/* !defined(MP_ERROR_PRINT_DEFINITION) */ >>> +#else >>> +/* defined(MP_ERROR_PRINT_DEFINITION) */ >>> + >>> +/** >>> + * 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. >>> + */ >>> + >>> +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d >>> +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d) >>> +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__) >>> + >>> +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name) >>> +#define mp_print_error_one mp_func_name(error_one) >>> +#define mp_print_error_stack mp_func_name(error_stack) >>> +#define mp_print_error mp_func_name(error) >>> +#define mp_print_common mp_func_name(recursion) >> Maybe we should align the assignments to be able to read it, like > I am pretty much able to read it as is, and moreover our code style > does not contain any kinds of such alignments. I explained that all > already in public chats, in private, in emails. Not going to describe > pros and cons again. > > I applied the alignment, and faced the problems right away - with equal > alignment for MP_CONCAT4_R, MP_CONCAT4, and MP_PRINT one of the lines > becomes too big, out of 80 symbols. On the other hand with not equal > alignments it would not look better than now. > > The mp_func_name() definitions with +2 tabs fall out of 80 as well, but > with +1 the space is too short - any a bit longer name will violate > the alignment in future causing either big unnecessary diff or alignment > violation. > >> #define MP_CONCAT4_R(a, b, c, d) a##b##c##d >> #define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d) >> #define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__) >> >> #define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name) >> #define mp_print_error_one mp_func_name(error_one) >> #define mp_print_error_stack mp_func_name(error_stack) >> #define mp_print_error mp_func_name(error) >> #define mp_print_common mp_func_name(recursion) >> >>> + >>> +static int >>> +mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth) >>> +{ >>> + int total = 0; >>> + MP_PRINT(total, "{"); >>> + if (depth <= 0) { >>> + MP_PRINT(total, "...}"); >>> + return total; >>> + } >>> + const char *field_to_key[MP_ERROR_MAX] = { >>> + /* MP_ERROR_TYPE = */ "\"type\": ", >>> + /* MP_ERROR_FILE = */ "\"file\": ", >>> + /* MP_ERROR_LINE = */ "\"line\": ", >>> + /* MP_ERROR_MESSAGE = */ "\"message\": ", >>> + /* MP_ERROR_ERRNO = */ "\"errno\": ", >>> + /* MP_ERROR_CODE = */ "\"code\": ", >>> + /* MP_ERROR_FIELDS = */ "\"fields\": ", >>> + }; >> Please use designited initializers in a readable way >> instead of this bloody mess >> >> const char *field_to_key[] = { >> [MP_ERROR_TYPE] = "\"type\": ", >> ... >> [MP_ERROR_FILE] = ..., >> ... >> [MP_ERROR_FIELDS] = "\"fields\": ", >> } > Once again - this is not a mess, this is our codestyle. If you > don't like it, you should talk to somebody responsible for our > SOP to change it. We can't write our code in individual code > styles, and silently hope to push the old style away eventually. > If you want changes, they need to go through our documentation > and the responsible person. This is Kirill Y., as I remember. > > Talking of [] = initializer - I didn't know you > can do that in C. Cool, thanks. Now it should be safer. > > I applied the alignments, but I won't push it into the master until, > of course, the branch gets all ACKs, *and* until we agree to either > align all new code, or don't align such places at all. In the latter > case I will return the old version. > > ==================== > > diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc > index fed2ce288..0b5e6bc96 100644 > --- a/src/box/mp_error.cc > +++ b/src/box/mp_error.cc > @@ -589,15 +589,16 @@ error_unpack_unsafe(const char **data) > * the actual output. > */ > > -#define MP_CONCAT4_R(a, b, c, d) a##b##c##d > -#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d) > -#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__) > +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d > +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d) > +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, \ > + __VA_ARGS__) > > -#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name) > -#define mp_print_error_one mp_func_name(error_one) > -#define mp_print_error_stack mp_func_name(error_stack) > -#define mp_print_error mp_func_name(error) > -#define mp_print_common mp_func_name(recursion) > +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name) > +#define mp_print_error_one mp_func_name(error_one) > +#define mp_print_error_stack mp_func_name(error_stack) > +#define mp_print_error mp_func_name(error) > +#define mp_print_common mp_func_name(recursion) > ==================== > > I don't know why the line above is screwed. In source it looks ok. > Probably due to '+' in the beginning, added by git. One another > reason against alignments - they will look shitty in git diff, show, > etc. I personally like the aligned version more, but it's up to you. LGTM > > ==================== > > static int > mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth) > @@ -609,13 +610,13 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth) > return total; > } > const char *field_to_key[MP_ERROR_MAX] = { > - /* MP_ERROR_TYPE = */ "\"type\": ", > - /* MP_ERROR_FILE = */ "\"file\": ", > - /* MP_ERROR_LINE = */ "\"line\": ", > - /* MP_ERROR_MESSAGE = */ "\"message\": ", > - /* MP_ERROR_ERRNO = */ "\"errno\": ", > - /* MP_ERROR_CODE = */ "\"code\": ", > - /* MP_ERROR_FIELDS = */ "\"fields\": ", > + [MP_ERROR_TYPE] = "\"type\": ", > + [MP_ERROR_FILE] = "\"file\": ", > + [MP_ERROR_LINE] = "\"line\": ", > + [MP_ERROR_MESSAGE] = "\"message\": ", > + [MP_ERROR_ERRNO] = "\"errno\": ", > + [MP_ERROR_CODE] = "\"code\": ", > + [MP_ERROR_FIELDS] = "\"fields\": ", > }; > --depth; > if (mp_typeof(**data) != MP_MAP) -- Serge Petrenko