[tarantool-patches] Re: [PATCH v1 1/2] box: export mpstream methods to core

[Vladislav Shpilevoy <v.shpilevoy@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 <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;
> +}