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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

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()).

>>> +#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.