From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 A0AE242EF5C for ; Fri, 19 Jun 2020 01:48:24 +0300 (MSK) References: <28de36d645dab72681f6678d1b0774d64fa323d3.1591548554.git.alexander.turenko@tarantool.org> <20200617175347.47de7la5fyrglucz@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: <87fd0e77-6964-b75a-3f44-d65741d69d76@tarantool.org> Date: Fri, 19 Jun 2020 00:48:23 +0200 MIME-Version: 1.0 In-Reply-To: <20200617175347.47de7la5fyrglucz@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Alexander Turenko Cc: tarantool-patches@dev.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_*() */ >>> +#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.