Re: [Tarantool-patches] [PATCH 09/20] net.box: rewrite request encoder in C

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 8952efb7bb39..49030aabea69 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -52,16 +52,34 @@

<...>

>  static inline size_t
> -netbox_prepare_request(lua_State *L, struct mpstream *stream, uint32_t r_type)
> +netbox_prepare_request(struct mpstream *stream, uint64_t sync,
> +		       enum iproto_type type)
>  {
> -	struct ibuf *ibuf = (struct ibuf *) lua_topointer(L, 1);
> -	uint64_t sync = luaL_touint64(L, 2);
> -
> -	mpstream_init(stream, ibuf, ibuf_reserve_cb, ibuf_alloc_cb,
> -		      luamp_error, L);
> -
>  	/* Remember initial size of ibuf (see netbox_encode_request()) */
> +	struct ibuf *ibuf = (struct ibuf *) stream->ctx;

1. There is a rule now that in the new code after unary operators we
do not put whitespaces. But here you do not need a cast. In C void * to
any other pointer works implicitly.

Here and in other new places.

> @@ -108,16 +126,15 @@ netbox_encode_request(struct mpstream *stream, size_t initial_size)
>  	mp_store_u32(fixheader, total_size - fixheader_size);
>  }
>  
> -static int
> -netbox_encode_ping(lua_State *L)
> +static void
> +netbox_encode_ping(lua_State *L, int idx, struct ibuf *ibuf, uint64_t sync)
>  {
> -	if (lua_gettop(L) < 2)
> -		return luaL_error(L, "Usage: netbox.encode_ping(ibuf, sync)");
> -
> +	(void) idx;
>  	struct mpstream stream;
> -	size_t svp = netbox_prepare_request(L, &stream, IPROTO_PING);
> +	mpstream_init(&stream, ibuf, ibuf_reserve_cb, ibuf_alloc_cb,
> +		      luamp_error, L);
> +	size_t svp = netbox_prepare_request(&stream, sync, IPROTO_PING);

2. mpstream_init and netbox_prepare_request in 100% cases go together.
Maybe you could move the init into netbox_prepare_request? It even takes
the stream pointer already. You would only need to pass L again.