Re: [Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Cyrill Gorcunov <gorcunov@gmail.com>]

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

On Mon, May 18, 2020 at 6:18 PM Serge Petrenko
<sergepetrenko@tarantool.org> wrote:
>
>
> 12.05.2020 02:46, Vladislav Shpilevoy пишет:
> > The patchset makes MP_EXT serializer for mp_snprint() and
> > mp_fprint() functions virtual.
> >
> > The patch implements one possible solution - return recursion
> > instead of mp_stack into the printers, and make external code
> > overload both mp_fprint() and mp_snprint(). The most
> > straightforward solution.
> >
> > However it is possible to avoid overloading both mp_fprint() and
> > mp_snprint(). Instead, msgpuck could expose just one virtual
> > method, which takes an opaque object with method print() having
> > signature like snprintf() and fprintf() - format and arguments.
> >
> > mp_snprint() would create that opaque object's print() method as
> > snprintf(). While mp_fprint() would create this object's print()
> > method as fprintf(). Drawback of the solution - ton of virtual
> > calls, because serializers tend to call print() a lot, on small
> > strings like ':', ', ', and so on. On the other hand, this should
> > simplify code in Tarantool, especially MP_ERROR serializer, where
> > code duplication was so big for mp_snprint_error() and
> > mp_fprint_error(), that I invented some kind of C template for it.
> >
> > With one virtual method, which accepts a virtual object it would
> > be just one mp_print_error().
> >
> > I decided to leave the final decision to a review discussion.
> > Should we keep it like it is done now, or make it even more
> > virtual like I described above, or probably there are other ideas.
> >
> > Branch: http://github.com/tarantool/msgpuck/tree/gerold103/mp_print_ext
> > Issue: https://github.com/tarantool/tarantool/issues/4719
> >
> > Vladislav Shpilevoy (2):
> >    Return recursion to mp_snprint() and mp_fprint()
> >    Make MP_EXT mp_snprint() and mp_fprint() customizable
> >
> >   msgpuck.c      | 156 ++++++++++++++++++++++++++++++++-----------------
> >   msgpuck.h      |  50 ++++++++++++++++
> >   test/msgpuck.c |  83 +++++++++++++++++++++++++-
> >   3 files changed, 234 insertions(+), 55 deletions(-)
>
>
> Hi! Thanks for the patchset! LGTM(including patch 1.5/2)
>
> --
> Serge Petrenko
>