Re: [Tarantool-patches] [PATCH] net.box: allow to store user-defined fields in future object

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

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 06e574cdf746..162ff6c82fb0 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
<...>

> +
> +static int
> +luaT_netbox_request_newindex(struct lua_State *L)
> +{
> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> +	struct mh_strnptr_t *h = request->index;
> +	size_t field_name_len;
> +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
> +	if (field_name == NULL)
> +		return luaL_error(L, "invalid index");
> +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> +	if (field_value_ref == LUA_REFNIL) {
> +		/* The field is set to nil. Delete it from the index. */

1. But it is not deleted. It is just ignored, which is fine, but
the comment is wrong then.

> +		if (h == NULL)
> +			return 0;
> +		mh_int_t k = mh_strnptr_find_inp(h, field_name,
> +						 field_name_len);
> +		int ref = (intptr_t)mh_strnptr_node(h, k)->val;
> +		luaL_unref(L, LUA_REGISTRYINDEX, ref);
> +		mh_strnptr_del(h, k, NULL);
> +		return 0;
> +	}
> +	if (h == NULL) {
> +		/* Lazily create the index on the first invocation. */
> +		h = request->index = mh_strnptr_new();
> +		if (h == NULL) {
> +			luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
> +			return luaL_error(L, "out of memory");
> +		}
> +	}
> +	/* Insert a reference to the new value into the index. */
> +	struct mh_strnptr_node_t node = {
> +		.str = field_name,
> +		.len = field_name_len,
> +		.hash = mh_strn_hash(field_name, field_name_len),

2. lua_hashstring() might be faster. You could reuse already
calculated hash value from Lua. Although for that 'field_name'
must be of exactly string type (lua_tolstring() works not only for
strings). So if you would make a more precise type check, you
could avoid hash calculation completely. Or use lua_hash() for
non-string values' strings. (But I might be wrong about how
lua_hashstring() works for non-string values, you would need to
double-check).

> +		.val = (void *)(intptr_t)field_value_ref,

3. Can you simply cast to void * right away? Why do you need the
intermediate intptr_t cast?

> +	};
> +	struct mh_strnptr_node_t old_node = { NULL, 0, 0, NULL };
> +	struct mh_strnptr_node_t *p_old_node = &old_node;
> +	if (mh_strnptr_put(h, &node, &p_old_node,
> +			   NULL) == mh_end(h)) {
> +		luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
> +		return luaL_error(L, "out of memory");
> +	}
> +	/* Drop the reference to the overwritten value. */
> +	if (p_old_node != NULL)
> +		luaL_unref(L, LUA_REGISTRYINDEX, (intptr_t)old_node.val);
> +	return 0;
> +}