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

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

On Thu, Aug 12, 2021 at 09:02:46PM +0300, Vladislav Shpilevoy wrote:
> > +
> > +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?

I reworked the patch to use a Lua table instead of mhash, as per
the suggestion by imun@, so these comments are not relevant anymore.
Please see v3:

https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025458.html