Re: [Tarantool-patches] [PATCH 2.5/3] merger: clean fiber-local Lua stack after next()

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Fri, Jun 19, 2020 at 12:48:23AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> On 17/06/2020 19:53, Alexander Turenko wrote:
> > Thanks for the careful review!
> > 
> >>> +	if (top >= 0)
> >>> +		lua_settop(L, top);
> >>
> >> 1. lua_settop() works fine even when top is -1. It basically means
> >> 'set top to the latest element' = 'leave the stack untouched'. I
> >> checked the implementation, should work.
> > 
> > I'm not sure we can lean on this. See, Lua 5.1 Reference Manual [1] states the
> > following about lua_settop():
> > 
> >  | Accepts any acceptable index, or 0, and sets the stack top to this index.
> > 
> > And defines an acceptable index as follows:
> > 
> >  | More formally, we define an acceptable index as follows:
> >  |
> >  |     (index < 0 && abs(index) <= top) ||
> >  |     (index > 0 && index <= stackspace)
> > 
> > However LuaJIT has the following condition:
> 
> You said a few lines above that we shouldn't rely on implementation
> specifics, and yet you appeal to it here.

I just show where Lua and LuaJIT allows more than the reference manual
guarantees to be successful.

> As I see, both the implementation, and the format definition of the
> valid index mean that lua_settop(-1) is no op. Means the same as
> lua_settop(lua_gettop()).

Let `top` be zero (empty stack) and `index` be -1. (index < 0 &&
abs(index) <= top) condition fails. I don't see any mistake in this
math.

> 
> >>> +#include <lua.h>           /* lua_*() */
> >>> +#include <lauxlib.h>       /* struct luaL_Reg */
> >>> +#include "lib/core/diag.h" /* struct error, diag_*() */
> >>> +#include "fiber.h"         /* fiber_self() */
> >>> +#include "lua/utils.h"     /* luaL_checkcdata() */
> >>> +#include "box/merger.h"    /* struct merge_source,
> >>> +			      merge_source_next() */
> >>
> >> 3. Do you really need these comments? Anyway they tend to outdate
> >> fast, because no one watches these comments when changes the code,
> >> uses some new functions from these files, etc.
> > 
> > I actively use them during the initial development: when I remove some
> > experimental code, I verify whether I should remove some header.
> > 
> > There is nothing bad if it becomes a bit outdated: some of headers used
> > more, some becomes unused. If one want to clean them up, those comments
> > will give an idea how to obtain possibly unused headers using simple
> > pattern matching. Then the list of possibly unused headers may be
> > verified by removing them and try to compile.
> > 
> > I find it convenient sometimes, so I would prefer to leave the comments
> > (if you don't strongly disagree).
> 
> I don't care about include comments much. I just warn you that
> it is not in our code style (AFAIK, but I didn't check), and if
> someone but you will change the merger code, the comments are likely
> to outdate and turn into just confusing text not meaning anything.

I asked teammates and several of them speak against it. So I'll remove
them.