[tarantool-patches] Re: [PATCH v1 1/2] box: export mpstream methods to core
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Aug 24 19:33:32 MSK 2018
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 <stdbool.h>
>
> #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 <COPYRIGHT HOLDER> ``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
> + * <COPYRIGHT HOLDER> 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 <stddef.h>
6. Why do you need stddef.h?
> +#include <assert.h>
> +#include <stdint.h>
> +#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;
> +}
More information about the Tarantool-patches
mailing list