From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 45890469710 for ; Tue, 19 May 2020 12:11:08 +0300 (MSK) Received: by mail-wm1-f65.google.com with SMTP id h4so2234458wmb.4 for ; Tue, 19 May 2020 02:11:08 -0700 (PDT) MIME-Version: 1.0 References: <8292d693-88a3-3c6c-a77e-2da51db0d7dc@tarantool.org> In-Reply-To: <8292d693-88a3-3c6c-a77e-2da51db0d7dc@tarantool.org> From: Cyrill Gorcunov Date: Tue, 19 May 2020 12:10:55 +0300 Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tml , Vladislav Shpilevoy Reviewed-by: Cyrill Gorcunov On Mon, May 18, 2020 at 6:18 PM Serge Petrenko wrote: > > > 12.05.2020 02:46, Vladislav Shpilevoy =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > 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 >