From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 1A9162A8E4 for ; Fri, 24 Aug 2018 12:33:38 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zxXWCvEq4XW7 for ; Fri, 24 Aug 2018 12:33:38 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9B7E22A847 for ; Fri, 24 Aug 2018 12:33:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/2] box: export mpstream methods to core References: <792834dabd4287aeb213202581de02b23a4c4557.1534508548.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 24 Aug 2018 19:33:32 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Cc: korablev@tarantool.org Hi! Thanks for the patch! See 8 comments below. 1. Please, do not send a patchset in different mail threads. On 17/08/2018 17:04, Kirill Shcherbatov wrote: > As we going to use mpstream not only in LUA, let's move this > API in tarantool core. > > Part of #3545. > --- > src/lua/msgpack.c | 85 +++---------------- > src/lua/msgpack.h | 53 +----------- > src/mpstream.h | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 248 insertions(+), 128 deletions(-) > create mode 100644 src/mpstream.h > > diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c > index acd860a..bbcb886 100644 > --- a/src/lua/msgpack.c > +++ b/src/lua/msgpack.c > @@ -50,46 +50,6 @@ luamp_error(void *error_ctx) > luaL_error(L, diag_last_error(diag_get())->errmsg); > } > > -void > -mpstream_init(struct mpstream *stream, void *ctx, > - luamp_reserve_f reserve, luamp_alloc_f alloc, > - luamp_error_f error, void *error_ctx) > -{ > - stream->ctx = ctx; > - stream->reserve = reserve; > - stream->alloc = alloc; > - stream->error = error; > - stream->error_ctx = error_ctx; > - mpstream_reset(stream); > -} > - > -void > -mpstream_reserve_slow(struct mpstream *stream, size_t size) > -{ > - stream->alloc(stream->ctx, stream->pos - stream->buf); > - stream->buf = (char *) stream->reserve(stream->ctx, &size); > - if (stream->buf == NULL) { > - diag_set(OutOfMemory, size, "mpstream", "reserve"); > - stream->error(stream->error_ctx); > - } > - stream->pos = stream->buf; > - stream->end = stream->pos + size; > -} > - > -void > -mpstream_reset(struct mpstream *stream) > -{ > - size_t size = 0; > - stream->buf = (char *) stream->reserve(stream->ctx, &size); > - if (stream->buf == NULL) { > - diag_set(OutOfMemory, size, "mpstream", "reset"); > - stream->error(stream->error_ctx); > - } > - stream->pos = stream->buf; > - stream->end = stream->pos + size; > -} > - > - > static uint32_t CTID_CHAR_PTR; > static uint32_t CTID_STRUCT_IBUF; > > @@ -112,10 +72,7 @@ luamp_encode_array(struct luaL_serializer *cfg, struct mpstream *stream, > uint32_t size) > { > (void) cfg; > - assert(mp_sizeof_array(size) <= 5); > - char *data = mpstream_reserve(stream, 5); > - char *pos = mp_encode_array(data, size); > - mpstream_advance(stream, pos - data); > + mpstream_encode_array(stream, size); > } > > void > @@ -123,10 +80,7 @@ luamp_encode_map(struct luaL_serializer *cfg, struct mpstream *stream, > uint32_t size) > { > (void) cfg; > - assert(mp_sizeof_map(size) <= 5); > - char *data = mpstream_reserve(stream, 5); > - char *pos = mp_encode_map(data, size); > - mpstream_advance(stream, pos - data); > + mpstream_encode_map(stream, size); > } > > void > @@ -134,10 +88,7 @@ luamp_encode_uint(struct luaL_serializer *cfg, struct mpstream *stream, > uint64_t num) > { > (void) cfg; > - assert(mp_sizeof_uint(num) <= 9); > - char *data = mpstream_reserve(stream, 9); > - char *pos = mp_encode_uint(data, num); > - mpstream_advance(stream, pos - data); > + mpstream_encode_uint(stream, num); > } > > void > @@ -145,10 +96,7 @@ luamp_encode_int(struct luaL_serializer *cfg, struct mpstream *stream, > int64_t num) > { > (void) cfg; > - assert(mp_sizeof_int(num) <= 9); > - char *data = mpstream_reserve(stream, 9); > - char *pos = mp_encode_int(data, num); > - mpstream_advance(stream, pos - data); > + mpstream_encode_int(stream, num); > } > > void > @@ -156,10 +104,7 @@ luamp_encode_float(struct luaL_serializer *cfg, struct mpstream *stream, > float num) > { > (void) cfg; > - assert(mp_sizeof_float(num) <= 5); > - char *data = mpstream_reserve(stream, 5); > - char *pos = mp_encode_float(data, num); > - mpstream_advance(stream, pos - data); > + mpstream_encode_float(stream, num); > } > > void > @@ -167,10 +112,7 @@ luamp_encode_double(struct luaL_serializer *cfg, struct mpstream *stream, > double num) > { > (void) cfg; > - assert(mp_sizeof_double(num) <= 9); > - char *data = mpstream_reserve(stream, 9); > - char *pos = mp_encode_double(data, num); > - mpstream_advance(stream, pos - data); > + mpstream_encode_double(stream, num); > } > > void > @@ -178,20 +120,14 @@ luamp_encode_str(struct luaL_serializer *cfg, struct mpstream *stream, > const char *str, uint32_t len) > { > (void) cfg; > - assert(mp_sizeof_str(len) <= 5 + len); > - char *data = mpstream_reserve(stream, 5 + len); > - char *pos = mp_encode_str(data, str, len); > - mpstream_advance(stream, pos - data); > + mpstream_encode_str(stream, str, len); > } > > void > luamp_encode_nil(struct luaL_serializer *cfg, struct mpstream *stream) > { > (void) cfg; > - assert(mp_sizeof_nil() <= 1); > - char *data = mpstream_reserve(stream, 1); > - char *pos = mp_encode_nil(data); > - mpstream_advance(stream, pos - data); > + mpstream_encode_nil(stream); > } > > void > @@ -199,10 +135,7 @@ luamp_encode_bool(struct luaL_serializer *cfg, struct mpstream *stream, > bool val) > { > (void) cfg; > - assert(mp_sizeof_bool(val) <= 1); > - char *data = mpstream_reserve(stream, 1); > - char *pos = mp_encode_bool(data, val); > - mpstream_advance(stream, pos - data); > + mpstream_encode_bool(stream, val); > } 2. All these functions are useless now and can be inlined in the places of usage. Please, do it. > > static enum mp_type > diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h > index bacf3e0..d8ad202 100644 > --- a/src/lua/msgpack.h > +++ b/src/lua/msgpack.h > @@ -34,6 +34,7 @@ > #include > > #include "utils.h" > +#include "mpstream.h" 3. It is enough to announce struct mpstream. You do not need to include the whole header. > > #if defined(__cplusplus) > extern "C" { > diff --git a/src/mpstream.h b/src/mpstream.h > new file mode 100644 > index 0000000..c8696f6 > --- /dev/null > +++ b/src/mpstream.h > @@ -0,0 +1,238 @@ > +#ifndef TARANTOOL_LUA_MPSTREAM_H_INCLUDED > +#define TARANTOOL_LUA_MPSTREAM_H_INCLUDED > +/* > + * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file. 4. 2018 > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ 5. Why do you need c++ protector, if this is a header-only lib? By the way, I do not think it is a true method. There is no point in making all these functions static inline. Please, add a C file and move these functions there. > + > +#include 6. Why do you need stddef.h? > +#include > +#include > +#include "msgpuck.h" > +#include "diag.h" > + > +/** > +* Ask the allocator to reserve at least size bytes. It can reserve > +* more, and update *size with the new size. > +*/ > +typedef void *(*mpstream_reserve_f)(void *ctx, size_t *size); 7. Excessive white spaces. The same below. > + > +/** Actually use the bytes. */ > +typedef void *(*mpstream_alloc_f)(void *ctx, size_t size); > + > +/** Actually use the bytes. */ > +typedef void (*mpstream_error_f)(void *error_ctx); > + > +struct mpstream { > + /** > + * When pos >= end, or required size doesn't fit in > + * pos..end range alloc() is called to advance the stream > + * and reserve() to get a new chunk. > + */ > + char *buf, *pos, *end; > + void *ctx; > + mpstream_reserve_f reserve; > + mpstream_alloc_f alloc; > + mpstream_error_f error; > + void *error_ctx; > +}; > + > +static inline void > +mpstream_reserve_slow(struct mpstream *stream, size_t size) 8. There was a reason, why reserve_slow is an independent function and reserve is static inline. This is why in most cases reserve just returns a current position, and rarely it falls down to alloc(). Please, return as it was. Same for advance. > +{ > + stream->alloc(stream->ctx, stream->pos - stream->buf); > + stream->buf = (char *) stream->reserve(stream->ctx, &size); > + if (stream->buf == NULL) { > + diag_set(OutOfMemory, size, "mpstream", "reserve"); > + stream->error(stream->error_ctx); > + } > + stream->pos = stream->buf; > + stream->end = stream->pos + size; > +}