From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 74AE9469719 for ; Mon, 12 Oct 2020 13:37:01 +0300 (MSK) Date: Mon, 12 Oct 2020 13:37:19 +0300 From: Alexander Turenko Message-ID: <20201012103719.fdzugbvvp4nfvcqi@tkn_work_nb> References: <20201011174755.GX18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201011174755.GX18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy It seems we look at this code from some very different positions. I have my patterns in the mind and you have your ones. WBR, Alexander Turenko. On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote: > Sasha, > > It is not full review, but Vlad asked me offline regarding a passage > below. So I dump our discussion results. > > On 11.10.20, Alexander Turenko wrote: > > > > > + int top = lua_gettop(L); > > This value can't be used, since can trigger Lua stack > reallocation, ergo this value will be invalidated. AFAIU, you rejected this comment in the next email? | This is not the address, but offset, so nevermind. > > > + > > + /* Calculate absolute value in the stack. */ > > At first, it was tough to me to understand the reason you transform the > given relative index to an absolute one, since there is everything > within for it. I finally got the issue after Vlad's > comments and another (more thorough) look to the sources. I believe it's > nice to drop a few words regarding it. Here are the key points (IMHO): > * whether index is less than zero, it is considered relative to the top > Lua stack slot > * when you obtain the function object to be called, top pointer is > incremented, so index ought to be adjusted respectively > > > + if (idx < 0) > > + idx = top + idx + 1; > > Well, is this math even correct? AFAICS, you copy the slot on the > top as a first argument for , right? So, > this is the original guest stack layout: > | nil | <- L->top > | ... | > | val | <- idx > And this is the resulting one: > | nil | <- L->top > | val | > | fun | <- old L->top > | ... | > | val | <- idx > > So, it looks like you need to subtract 1 instead of adding it, since > is negative. Feel free to correct me if I'm bad in this math. > > Anyway, technically, you don't need to calculate the absolute value by > yourself, just adjust the offset to the given slot. I guess the > following line is enough (with the verbose comment I mentioned above): > | idx -= idx < 0; Hm. Hmmm. The math is correct. We have the linear dependency, so it seems we can just verify one negative idx and others should be good too for any valid composition of idx and top. top: 4; idx: -1 -> 4 -- ok top: 4; idx: -2 -> 3 -- ok This snippet is used several times across tarantool code base: say, in luaL_checkcdata(). Let's show the sketchy code: | int top = gettop(L); | | int rc = lua_pcall(<...>); | lua_settop(L, top); | It is much, MUCH better than doing all those lua_pop(L, 1) or lua_pop(L, 2) depending on lua_pcall() return value. Now, look at another schetchy code: | int idx = absolute(idx); | | lua_pushvalue(L, idx); It again much, MUCH better than doing all those idx + 1 or -1 or even -2 depending on how top is changed. > > > + > > + assert(luaT_tuple_encode_table_ref != LUA_NOREF); > > + lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref); > > + assert(lua_isfunction(L, -1)); > > + > > + lua_pushvalue(L, idx); > > There is also another way: simply leave the comment prior to > call and pass the proper index as an argument > | lua_pushvalue(L, idx - (idx < 0)); It'll be in the next commit. I don't want to play this game and still think that it is much better to just use an absolute index.