From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4C0F04429E1 for ; Fri, 19 Jun 2020 10:42:10 +0300 (MSK) Date: Fri, 19 Jun 2020 10:41:22 +0300 From: Alexander Turenko Message-ID: <20200619074122.7ijjpgqkmqtdbxpq@tkn_work_nb> References: <28de36d645dab72681f6678d1b0774d64fa323d3.1591548554.git.alexander.turenko@tarantool.org> <20200617175347.47de7la5fyrglucz@tkn_work_nb> <87fd0e77-6964-b75a-3f44-d65741d69d76@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87fd0e77-6964-b75a-3f44-d65741d69d76@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2.5/3] merger: clean fiber-local Lua stack after next() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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_*() */ > >>> +#include /* 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.