[Tarantool-patches] [PATCH 2.5/3] merger: clean fiber-local Lua stack after next()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jun 19 01:48:23 MSK 2020
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.
More information about the Tarantool-patches
mailing list