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 38071469719 for ; Mon, 12 Oct 2020 14:01:43 +0300 (MSK) Date: Mon, 12 Oct 2020 13:51:05 +0300 From: Igor Munkin Message-ID: <20201012105105.GZ18920@tarantool.org> References: <20201011174755.GX18920@tarantool.org> <20201012103719.fdzugbvvp4nfvcqi@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201012103719.fdzugbvvp4nfvcqi@tkn_work_nb> 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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sasha, On 12.10.20, Alexander Turenko wrote: > It seems we look at this code from some very different positions. I have > my patterns in the mind and you have your ones. Totally agree here. > > WBR, Alexander Turenko. > > On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote: > > Sasha, > > > > AFAIU, you rejected this comment in the next email? > > | This is not the address, but offset, so nevermind. Yes, I did. > > > > > > + > > > + /* 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 I hope, we are on the same page here, aren't we? > > > > > + 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 I re-checked these calculations manually, so both approaches are fine. > > 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. I don't want to argue about which approach is *much better*. This is the only spot I can agree with you. Other ones above are just based on the patterns we have in our minds. However, despite the patters we have in our minds, such non-trivial (at least to me) places should be described with a nice comment. -- Best regards, IM