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

[Igor Munkin <imun@tarantool.org>]

Sasha,

On 12.10.20, Alexander Turenko wrote:
> It seems we look at this code from some very different positions. I have
> my patterns in the mind and you have your ones.

Totally agree here.

> 
> WBR, Alexander Turenko.
> 
> On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote:
> > Sasha,
> > 

<snipped>

> 
> AFAIU, you rejected this comment in the next email?
> 
>  | This is not the address, but offset, so nevermind.

Yes, I did.

> 
> > 
> > > +
> > > +	/* 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

I hope, we are on the same page here, aren't we?

> > 
> > > +	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;
> 
> Hm. Hmmm.
> 
> The math is correct. We have the linear dependency, so it seems we can
> just verify one negative idx and others should be good too for any valid
> composition of idx and top.
> 
> top: 4; idx: -1 -> 4 -- ok
> top: 4; idx: -2 -> 3 -- ok

I re-checked these calculations manually, so both approaches are fine.

> 
> This snippet is used several times across tarantool code base: say, in
> luaL_checkcdata().
> 
> Let's show the sketchy code:
> 
>  | int top = gettop(L);
>  | <some stack manipulations>
>  | int rc = lua_pcall(<...>);
>  | lua_settop(L, top);
>  | <handle rc>
> 
> It is much, MUCH better than doing all those lua_pop(L, 1) or
> lua_pop(L, 2) depending on lua_pcall() return value.
> 
> Now, look at another schetchy code:
> 
>  | int idx = absolute(idx);
>  | <doing some stack manipulations>
>  | lua_pushvalue(L, idx);
> 
> It again much, MUCH better than doing all those idx + 1 or -1 or even -2
> depending on how top is changed.
> 
> > 
> > > +
> > > +	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));
> 
> It'll be <idx - 2 * (idx < 0)> in the next commit. I don't want to play
> this game and still think that it is much better to just use an
> absolute index.

I don't want to argue about which approach is *much better*. This is the
only spot I can agree with you. Other ones above are just based on the
patterns we have in our minds. However, despite the patters we have in
our minds, such non-trivial (at least to me) places should be described
with a nice comment.

-- 
Best regards,
IM