From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer Date: Tue, 12 May 2020 22:38:25 +0200 [thread overview] Message-ID: <6c8dc667-43ce-997b-1b1f-cf725f05a2f8@tarantool.org> (raw) In-Reply-To: <20200512175205.GF2219@grain> 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 [<number>] = <value> 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. ==================== 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)
next prev parent reply other threads:[~2020-05-12 20:38 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-11 23:45 [Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error Vladislav Shpilevoy 2020-05-11 23:45 ` [Tarantool-patches] [PATCH 1/5] msgpuck: bump version to enable extension printer Vladislav Shpilevoy 2020-05-12 17:34 ` Cyrill Gorcunov 2020-05-11 23:45 ` [Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer Vladislav Shpilevoy 2020-05-12 15:13 ` Cyrill Gorcunov 2020-05-12 20:30 ` Vladislav Shpilevoy 2020-05-12 20:56 ` Cyrill Gorcunov 2020-05-12 17:35 ` Cyrill Gorcunov 2020-05-11 23:45 ` [Tarantool-patches] [PATCH 3/5] uuid: provide MP_UUID " Vladislav Shpilevoy 2020-05-12 17:36 ` Cyrill Gorcunov 2020-05-11 23:45 ` [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR " Vladislav Shpilevoy 2020-05-12 17:52 ` Cyrill Gorcunov 2020-05-12 20:38 ` Vladislav Shpilevoy [this message] 2020-05-12 21:27 ` Cyrill Gorcunov 2020-05-18 15:24 ` Serge Petrenko 2020-05-13 12:31 ` Nikita Pettik 2020-05-13 22:10 ` Vladislav Shpilevoy 2020-05-14 2:32 ` Nikita Pettik 2020-05-14 21:28 ` Vladislav Shpilevoy 2020-05-19 13:21 ` Nikita Pettik 2020-05-20 21:57 ` Vladislav Shpilevoy 2020-05-19 11:51 ` Alexander Turenko 2020-05-19 20:48 ` Vladislav Shpilevoy 2020-05-11 23:45 ` [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers Vladislav Shpilevoy 2020-05-12 17:52 ` Cyrill Gorcunov 2020-05-13 21:06 ` Nikita Pettik 2020-05-13 21:48 ` Vladislav Shpilevoy 2020-05-14 2:24 ` Nikita Pettik 2020-05-14 21:27 ` Vladislav Shpilevoy 2020-05-19 12:11 ` Alexander Turenko 2020-05-19 20:48 ` Vladislav Shpilevoy 2020-05-19 13:23 ` Nikita Pettik 2020-05-18 15:25 ` [Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error Serge Petrenko 2020-05-21 18:25 ` Alexander Turenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6c8dc667-43ce-997b-1b1f-cf725f05a2f8@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox