Re: [Tarantool-patches] [PATCH 11/20] net.box: rewrite response decoder in C

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Jul 30, 2021 at 12:39:17AM +0200, Vladislav Shpilevoy wrote:
> On 23.07.2021 13:07, Vladimir Davydov via Tarantool-patches wrote:
> > +/**
> > + * Decode Tarantool response body consisting of single
> > + * IPROTO_DATA key into Lua table.
> > + * @param L Lua stack to push result on.
> > + * @param data MessagePack.
> > + * @retval Lua table
> 
> 1. It is not necessary to write these doxygen comments for each
> param really if you think they are obvious. The only 'rule' is to
> comment the function on its first declaration. Up to you of
> course. Not a rule.

You're right. Removed all the doxygen.

> > +static void
> > +netbox_decode_value(struct lua_State *L, const char **data,
> > +		    const char *data_end, struct tuple_format *format)
> > +{
> > +	(void) data_end;
> > +	(void) format;
> > +	netbox_skip_to_data(data);
> > +	uint32_t count = mp_decode_array(data);
> > +	for (uint32_t i = 0; i < count; ++i) {
> > +		if (i == 0)
> > +			luamp_decode(L, cfg, data);
> > +		else
> > +			mp_next(data);
> 
> 2. Is the compiler able to turn this into?:
> 
> 		...
> 		if (count == 0)
> 			return lua_pushnil(L);
> 
> 		luamp_decode(L, cfg, data);
> 		for (uint32_t i = 1; i < count; ++i)
> 			mp_next(data);
> 	}
> 
> And why not do it right away? It looks shorter and a bit
> simpler IMO. Up to you.

Looks better, reworked, thanks.

> > +static void
> > +netbox_decode_tuple(struct lua_State *L, const char **data,
> > +		    const char *data_end, struct tuple_format *format)
> > +{
> > +	(void) data_end;
> > +	netbox_skip_to_data(data);
> > +	uint32_t count = mp_decode_array(data);
> > +	for (uint32_t i = 0; i < count; ++i) {
> > +		const char *begin = *data;
> > +		mp_next(data);
> > +		if (i > 0)
> > +			continue;
> > +		struct tuple *tuple = box_tuple_new(format, begin, *data);
> > +		if (tuple == NULL)
> > +			luaT_error(L);
> > +		luaT_pushtuple(L, tuple);
> > +	}
> > +	if (count == 0)
> > +		lua_pushnil(L);
> 
> 3. Ditto. Wouldn't it be shorter and a bit easier to read by doing this?:
> 
> 		...
> 		if (count == 0)
> 			return lua_pushnil(L);
> 
> 		struct tuple *tuple = box_tuple_new(format, begin, *data);
> 		if (tuple == NULL)
> 			luaT_error(L);
> 		luaT_pushtuple(L, tuple);
> 		for (uint32_t i = 1; i < count; ++i)
> 			mp_next(data);
> 	}
> 
> Up to you.

Done.

The incremental diff is below. The full patch is available by the link:

https://github.com/tarantool/tarantool/commit/1087c1e687284efa4e39ce67fc4bea67265b7f58
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 2d2562550752..ac9052de286c 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -664,7 +664,7 @@ netbox_encode_method(struct lua_State *L)
 	return 0;
 }
 
-/**
+/*
  * This function handles a response that is supposed to have an empty body
  * (e.g. IPROTO_PING result). It doesn't decode anything per se. Instead it
  * simply pushes nil to Lua stack and advances the data ptr to data_end.
@@ -678,7 +678,7 @@ netbox_decode_nil(struct lua_State *L, const char **data,
 	lua_pushnil(L);
 }
 
-/**
+/*
  * This helper skips a MessagePack map header and IPROTO_DATA key so that
  * *data points to the actual response content.
  */
@@ -695,12 +695,9 @@ netbox_skip_to_data(const char **data)
 	(void)key;
 }
 
-/**
- * Decode Tarantool response body consisting of single
- * IPROTO_DATA key into Lua table.
- * @param L Lua stack to push result on.
- * @param data MessagePack.
- * @retval Lua table
+/*
+ * Decodes Tarantool response body consisting of single IPROTO_DATA key into
+ * a Lua table and pushes the table to Lua stack.
  */
 static void
 netbox_decode_table(struct lua_State *L, const char **data,
@@ -712,12 +709,9 @@ netbox_decode_table(struct lua_State *L, const char **data,
 	luamp_decode(L, cfg, data);
 }
 
-/**
+/*
  * Same as netbox_decode_table, but only decodes the first element of the
- * table, skipping the rest. Used to decode index.count() call result.
- * @param L Lua stack to push result on.
- * @param data MessagePack.
- * @retval count or nil.
+ * table, skipping the rest.
  */
 static void
 netbox_decode_value(struct lua_State *L, const char **data,
@@ -727,20 +721,15 @@ netbox_decode_value(struct lua_State *L, const char **data,
 	(void)format;
 	netbox_skip_to_data(data);
 	uint32_t count = mp_decode_array(data);
-	for (uint32_t i = 0; i < count; ++i) {
-		if (i == 0)
-			luamp_decode(L, cfg, data);
-		else
-			mp_next(data);
-	}
 	if (count == 0)
-		lua_pushnil(L);
+		return lua_pushnil(L);
+	luamp_decode(L, cfg, data);
+	for (uint32_t i = 1; i < count; ++i)
+		mp_next(data);
 }
 
-/**
- * Decode IPROTO_DATA into tuples array.
- * @param L Lua stack to push result on.
- * @param data MessagePack.
+/*
+ * Decodes IPROTO_DATA into a tuple array and pushes the array to Lua stack.
  */
 static void
 netbox_decode_data(struct lua_State *L, const char **data,
@@ -760,12 +749,9 @@ netbox_decode_data(struct lua_State *L, const char **data,
 	}
 }
 
-/**
- * Decode Tarantool response body consisting of single
- * IPROTO_DATA key into array of tuples.
- * @param L Lua stack to push result on.
- * @param data MessagePack.
- * @retval Tuples array.
+/*
+ * Decodes Tarantool response body consisting of single IPROTO_DATA key into
+ * tuple array and pushes the array to Lua stack.
  */
 static void
 netbox_decode_select(struct lua_State *L, const char **data,
@@ -776,12 +762,9 @@ netbox_decode_select(struct lua_State *L, const char **data,
 	netbox_decode_data(L, data, format);
 }
 
-/**
+/*
  * Same as netbox_decode_select, but only decodes the first tuple of the array,
  * skipping the rest.
- * @param L Lua stack to push result on.
- * @param data MessagePack.
- * @retval Tuple or nil.
  */
 static void
 netbox_decode_tuple(struct lua_State *L, const char **data,
@@ -790,18 +773,16 @@ netbox_decode_tuple(struct lua_State *L, const char **data,
 	(void)data_end;
 	netbox_skip_to_data(data);
 	uint32_t count = mp_decode_array(data);
-	for (uint32_t i = 0; i < count; ++i) {
-		const char *begin = *data;
-		mp_next(data);
-		if (i > 0)
-			continue;
-		struct tuple *tuple = box_tuple_new(format, begin, *data);
-		if (tuple == NULL)
-			luaT_error(L);
-		luaT_pushtuple(L, tuple);
-	}
 	if (count == 0)
-		lua_pushnil(L);
+		return lua_pushnil(L);
+	const char *begin = *data;
+	mp_next(data);
+	struct tuple *tuple = box_tuple_new(format, begin, *data);
+	if (tuple == NULL)
+		luaT_error(L);
+	luaT_pushtuple(L, tuple);
+	for (uint32_t i = 1; i < count; ++i)
+		mp_next(data);
 }
 
 /** Decode optional (i.e. may be present in response) metadata fields. */