From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 11DEC41C5DB for ; Wed, 17 Jun 2020 20:54:34 +0300 (MSK) Date: Wed, 17 Jun 2020 20:53:47 +0300 From: Alexander Turenko Message-ID: <20200617175347.47de7la5fyrglucz@tkn_work_nb> References: <28de36d645dab72681f6678d1b0774d64fa323d3.1591548554.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 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: | LUA_API void lua_settop(lua_State *L, int idx) | { | if (idx >= 0) { | <...> | } else { | api_check(L, -(idx+1) <= (L->top - L->base)); | L->top += idx+1; /* Shrinks top (idx < 0). */ | } | } The api_check() call allows a negative index to point to a stack slot below the current top by 1. It looks as the implementation detail. I was wonder from where the condition comes. It seems, LuaJIT borrows it from PUC-Rio Lua 5.1 and it was introduced in [2] right with this extra +1. NB: Widely used index2addr() LuaJIT internal function has the following check for a negative index: | api_check(L, idx != 0 && -idx <= L->top - L->base); And index2addr() is used in lua_toboolean(), which accepts an acceptable index according to the manual. I would better follow the API rather then lean on implementation details and provide a comment with clarification why it works. [1]: https://www.lua.org/manual/5.1/manual.html [2]: https://github.com/lua/lua/commit/255052b6c6a1b109c93b104c03de8968b56dcd5a > > /* Handle incorrect results count. */ > > if (nresult != 2) { > > - diag_set(IllegalParams, "Expected , got %d " > > + diag_set(IllegalParams, "Expected , , got %d " > > 2. Unnecessary diff. box-tap/gh-4954-merger-via-c.test.lua has three 'bad gen function' test cases and all expected errors in them are structured in the same way. It would be strange to verify an that an error message contains a typo (especially, when only one of them has it). > > +#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). > > +struct tuple; > > 4. If some merger API returns tuples, I guess it is merger's header > responsibility to announce all the needed structures. And it does. > So why do you need it here? It sounds reasonable: I cannot use merge source methods without at least opaque definition. Removed the declaration and added 'struct tuple (opaque)' to the '#include "box/merger.h"' comment. Also fixed comment style: #include "box/merger.h" /* struct merge_source, - merge_source_next() */ + * merge_source_next(), + * struct tuple (opaque) + */ > > +/** > > + * Call merge_source_next() virtual method of a merge source. > > + * > > + * The purpose of this function is to verify whether the > > + * fiber-local Lua stack is properly cleaned after > > + * merge_source_next() call on the passed merge source. > > + * > > + * @param merge_source a merge source to call > > + * merge_source_next() on it > > + * > > + * @retval is_next_ok whether the call is successful > > + * @retval err_msg error message from the call or nil > > + * @retval is_stack_even whether the fiber-local Lua stack is > > + * even after the call > > 5. This function takes L, and returns an int. These things are > details of how L is changed. So they can't be described like > that. The function has the Lua API and I want to describe it in Lua terms: it accepts a merge source and returns three values. However I agree that I should not use Doxygen markup for this sake, because it will contradict with actual C definition. Since we have no formal agreement how to document Lua and Lua/C functions (and tooling around it), it seems I should not use any special markup and just describe parameters and return values in a free form. I wrote it this way: | * The function is to be called from Lua. Lua API is the | * following: | * | * Parameters: | * | * - merge_source A merge source object to call | * merge_source_next() on it. | * | * Return values: | * | * - is_next_ok Whether the call is successful. | * - err_msg Error message from the call or nil. | * - is_stack_even Whether the fiber-local Lua stack is | * even after the call. Note: Vlad points me a couple of examples that describe Lua API in Lua/C terms: | /** | * Push a message using a protocol, depending on a session type. | * @param L Lua state. First argument on the stack is data to | * push. | * @retval 1 Success, true is pushed. | * @retval 2 Error. Nil and error object are pushed. | */ | static int | lbox_session_push(struct lua_State *L) | /** | * Create a new SWIM instance. SWIM is not created via FFI, | * because this operation yields. | * @retval 1 Success. A SWIM instance pointer is on the stack. | * @retval 2 Error. Nil and an error object are pushed. | */ | static int | lua_swim_new(struct lua_State *L) I'm not very comfortable with this way, the reasons are the following: - Just not so intuitive as a description in usual Lua terms. - Usually amount of returned values is not most important information, but, say, whether a first one is nil is important. We should point a user a way to use the API right in the API description: success case is when the first retval is not nil in the first place, not when a function returns one value. - It requires some knowledge about Lua/C API and so it is not acceptable as a source to generate user-visible API documentation for public Lua methods. - It will change if a function will be reimplemented using pure Lua / LuaJIT FFI despite that an actual API (a way to call and use) will not be changed. > Also, please, start sentences from capital letters, and end with > a dot. It is really hard to read the parameter comments above, they > look like one big piece of monolothic text. Okay, fixed. > > + -- FIXME: Enable after gh-5048: ('non-array tuple in a buffer > > + -- leads to assertion fail'). > > 6. The file is currently called gh-4954-merger-via-c.test.lua. So > will you change the file name, when the test is enabled? Will 5048 > get its own test, or it will be covered by this file without any > file name changes? It is one of test cases for gh-4954, but it is blocked by gh-5048. I'll enable it here with gh-5048 and will add a specific test case either to box-tap/merger.test.lua (because it already has truncated buffer cases) or as a separate file (not sure what is actually better, but a formal process requires the latter). The gh-5048 case will not check stack size (just error message), call a Lua/C procedure and so: it will be easier to understand and to verify it separately from gh-4954. I'll send a fix when this patchset will land to master to don't forget to uncomment the case here. ---- During self-review of the changes found and fixed two typos: diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua index 963b5825a..9fbb0919a 100755 --- a/test/box-tap/gh-4954-merger-via-c.test.lua +++ b/test/box-tap/gh-4954-merger-via-c.test.lua @@ -4,13 +4,13 @@ -- gh-4954: The fiber-local Lua stack should be even after -- merge_source_next() call from C code. -- --- See test/box-tap/merge_source.c for more information. +-- See test/box-tap/check_merge_source.c for more information. -- local fio = require('fio') -- Use BUILDDIR passed from test-run or cwd when run w/o --- test-run to find test/box-tap/merge_source.{so,dylib}. +-- test-run to find test/box-tap/check_merge_source.{so,dylib}. local build_path = os.getenv('BUILDDIR') or '.' package.cpath = fio.pathjoin(build_path, 'test/box-tap/?.so' ) .. ';' .. fio.pathjoin(build_path, 'test/box-tap/?.dylib') .. ';' .. And also commented the unused function to make luacheck happy (and simplify luacheck integration patchset rebasing if it'll land into master after this patchset): diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua index 59ff731a5..9fbb0919a 100755 --- a/test/box-tap/gh-4954-merger-via-c.test.lua +++ b/test/box-tap/gh-4954-merger-via-c.test.lua @@ -42,12 +42,16 @@ local function bad_buffer() return 1, buf end +-- FIXME: Enable after gh-5048: ('non-array tuple in a buffer +-- leads to assertion fail'). +--[[ local function bad_tuple_in_buffer() local tuple = 1 local buf = buffer.ibuf() msgpack.encode({tuple}, buf) return 1, buf end +]]-- local function empty_buffer(_, state) if state ~= nil then