Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Igor Munkin <imun@tarantool.org>]

Sasha,

It is not full review, but Vlad asked me offline regarding a passage
below. So I dump our discussion results.

On 11.10.20, Alexander Turenko wrote:

<snipped>

> +	int top = lua_gettop(L);

This value can't be used, since <lua_rawgeti> can trigger Lua stack
reallocation, ergo this value will be invalidated.

> +
> +	/* Calculate absolute value in the stack. */

At first, it was tough to me to understand the reason you transform the
given relative index to an absolute one, since there is everything
within <lua_pushvalue> for it. I finally got the issue after Vlad's
comments and another (more thorough) look to the sources. I believe it's
nice to drop a few words regarding it. Here are the key points (IMHO):
* whether index is less than zero, it is considered relative to the top
  Lua stack slot
* when you obtain the function object to be called, top pointer is
  incremented, so index ought to be adjusted respectively

> +	if (idx < 0)
> +		idx = top + idx + 1;

Well, is this math even correct? AFAICS, you copy the <idx> slot on the
top as a first argument for <luaT_tuple_encode_table_ref>, right? So,
this is the original guest stack layout:
| nil | <- L->top
| ... |
| val | <- idx
And this is the resulting one:
| nil | <- L->top
| val |
| fun | <- old L->top
| ... |
| val | <- idx

So, it looks like you need to subtract 1 instead of adding it, since
<idx> is negative. Feel free to correct me if I'm bad in this math.

Anyway, technically, you don't need to calculate the absolute value by
yourself, just adjust the offset to the given slot. I guess the
following line is enough (with the verbose comment I mentioned above):
| idx -= idx < 0;

> +
> +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> +	assert(lua_isfunction(L, -1));
> +
> +	lua_pushvalue(L, idx);

There is also another way: simply leave the comment prior to
<lua_pushvalue> call and pass the proper index as an argument
| lua_pushvalue(L, idx - (idx < 0));

> +
> +	int rc = luaT_call(L, 1, 0);

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM