From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0BB28469719 for ; Sun, 11 Oct 2020 20:58:32 +0300 (MSK) Date: Sun, 11 Oct 2020 20:47:55 +0300 From: Igor Munkin Message-ID: <20201011174755.GX18920@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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, 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. > + > + /* 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; > + > + 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)); > + > + int rc = luaT_call(L, 1, 0); > -- > 2.25.0 > -- Best regards, IM