From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B38BD988B5C; Tue, 23 Jan 2024 15:40:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B38BD988B5C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1706013602; bh=m2l333lPVfzdtvXIEK0NQ76v6yCDp3VmqNApU0y9kJ4=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=asrV5eOkU+AFQhq53r4tIIGnE/VOQF6fQLVl1OUF0MNU9q445Ro8PhKCnl69U8ILw lP3dNergy6D/jgqmWKKetS7T2ISVEFC8TlwFh0MoPSNZjEaZdQgrg3m7ZTBnv8w80I LlM2AZl2lYIS+WHBuZtjgIxZ2XkArt8JSLRarujQ= Received: from smtp16.i.mail.ru (smtp16.i.mail.ru [95.163.41.69]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0BD5C988B5C for ; Tue, 23 Jan 2024 15:40:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0BD5C988B5C Received: by smtp16.i.mail.ru with esmtpa (envelope-from ) id 1rSG4P-00EeIn-0Q; Tue, 23 Jan 2024 15:40:01 +0300 Date: Tue, 23 Jan 2024 15:35:47 +0300 To: Sergey Bronnikov Message-ID: References: <6dc8864ddf6d90860cf96dff9c5d784a20595b62.1705661401.git.skaplun@tarantool.org> <0c05d0d4-94d6-4efe-a89f-2a5071561c0b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0c05d0d4-94d6-4efe-a89f-2a5071561c0b@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD936B5060B5AFFD5314FC64B4F3DF4614E38FCF171E63F8145182A05F538085040F1D871803DB8570EA1B2F06A4922707735305D6BF1A1D5647E25A85A8988BE65 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721AF84DC1D70954DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C8DFB935205A313D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D865306525FE3C5406E4E50CB27C409BDB117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C088D2E8BEBF93D4B068655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C8BDE37D78FCB031643847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5E11EE9C3B8F65F2270A482DD8DA39862800696E7FB627DA2F87CCE6106E1FC07E67D4AC08A07B9B02D01283D1ACF37BACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFA76B25AA103F4C0FC1C6483C9E59CE69B6F62ABDCD6AF3B36ACEECFE7D8D7444BC84317DE62561534BEAD515961AA0A76C5D70594C86FD0E55BAB7E86C41A119E48CAC7CA610320002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGSQVxX8i/5W9Tb695HIhnw== X-DA7885C5: 8A897590AAD6B1A1239967AB460DE7F3C67802149E45CFC2B0F7BF0085C6A6EB262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE338C47F3A4BF73B9E1C5FB0A9138E61DF00FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 02/25] test: separate LuaJIT helpers from ffi_util.inc X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the review! Added the comments to the fucntions and force-pushed the branch. See also my answers below. On 23.01.24, Sergey Bronnikov wrote: > Hi, Sergey! > > thanks for the patch! > > On 1/19/24 14:32, Sergey Kaplun wrote: > > This patch moves common helpers from > > into separate files by analogy with . The > Why do you need to move to separate files? What is an idea behind it? I suppose that this is "common" helpers with a bit different functionality. As mentioned I tried to respect the existed code style of the suite here (i.e., it is done by analogy with ). All of these files return just a single function to use inside a test. > > `include()` helper isn't touched since it is used in system dependend > > tests, which won't be modified for now. > > > > Part of tarantool/tarantool#9398 > > --- > > test/LuaJIT-tests/common/fails.lua | 3 +++ > > test/LuaJIT-tests/common/ffi/checkfail.lua | 10 +++++++++ > > test/LuaJIT-tests/common/ffi/checktypes.lua | 11 ++++++++++ > > test/LuaJIT-tests/common/ffi_util.inc | 23 --------------------- > > 4 files changed, 24 insertions(+), 23 deletions(-) > > create mode 100644 test/LuaJIT-tests/common/fails.lua > > create mode 100644 test/LuaJIT-tests/common/ffi/checkfail.lua > > create mode 100644 test/LuaJIT-tests/common/ffi/checktypes.lua > > > > diff --git a/test/LuaJIT-tests/common/fails.lua b/test/LuaJIT-tests/common/fails.lua > > new file mode 100644 > > index 00000000..d555a2b5 > > --- /dev/null > > +++ b/test/LuaJIT-tests/common/fails.lua > > @@ -0,0 +1,3 @@ > > +return function(f, ...) > > + if pcall(f, ...) ~= false then error("failure expected", 2) end > > +end > > diff --git a/test/LuaJIT-tests/common/ffi/checkfail.lua b/test/LuaJIT-tests/common/ffi/checkfail.lua > > new file mode 100644 > > index 00000000..60d8449a > > --- /dev/null > > +++ b/test/LuaJIT-tests/common/ffi/checkfail.lua > > @@ -0,0 +1,10 @@ > > +local ffi = require("ffi") > > + > please add a comment with function's description Added the following comment: =================================================================== diff --git a/test/LuaJIT-tests/common/ffi/checkfail.lua b/test/LuaJIT-tests/common/ffi/checkfail.lua index 60d8449a..d3ca74e8 100644 --- a/test/LuaJIT-tests/common/ffi/checkfail.lua +++ b/test/LuaJIT-tests/common/ffi/checkfail.lua @@ -1,5 +1,9 @@ local ffi = require("ffi") +-- Checker that takes an array of strings that should represent +-- different invalid CTypes (a more common pattern). Also, the +-- second argument may be also the `loadstring` function to check +-- invalid literals or `ffi.cdef` to check invalid C definitions. return function(t, f) f = f or ffi.typeof for i=1,1e9 do =================================================================== > > +return function(t, f) > > + f = f or ffi.typeof > > + for i=1,1e9 do > > + local tp = t[i] > > + if not tp then break end > > + assert(pcall(f, tp) == false, tp) > > + end > > +end > > diff --git a/test/LuaJIT-tests/common/ffi/checktypes.lua b/test/LuaJIT-tests/common/ffi/checktypes.lua > > new file mode 100644 > > index 00000000..6d748e3b > > --- /dev/null > > +++ b/test/LuaJIT-tests/common/ffi/checktypes.lua > > @@ -0,0 +1,11 @@ > > +local ffi = require("ffi") > > + > please add a comment with function's description =================================================================== diff --git a/test/LuaJIT-tests/common/ffi/checktypes.lua b/test/LuaJIT-tests/common/ffi/checktypes.lua index 6d748e3b..c995d667 100644 --- a/test/LuaJIT-tests/common/ffi/checktypes.lua +++ b/test/LuaJIT-tests/common/ffi/checktypes.lua @@ -1,5 +1,9 @@ local ffi = require("ffi") +-- Checker that takes an array with the following each 3 elements: +-- 1) Sizeof for the given C type to be checked. +-- 2) Alignof for the given C type to be checked. +-- 3) String representing the C type. return function(t) for i=1,1e9,3 do local tp = t[i+2] =================================================================== > > +return function(t) > > + for i=1,1e9,3 do > > + local tp = t[i+2] > > + if not tp then break end > > + local id = ffi.typeof(tp) > > + assert(ffi.sizeof(id) == t[i], tp) > > + assert(ffi.alignof(id) == t[i+1], tp) > > Why test function contains asserts()? Wouldn't be better to return > false/true and raise assert in test itself? I have no perporse to refactor the whole suite. Also, I suppose there are much more to do ;). `expect_error()` raises the error to, so see no differences here. > > same questions below > > > + end > > +end > > diff --git a/test/LuaJIT-tests/common/ffi_util.inc b/test/LuaJIT-tests/common/ffi_util.inc > > index 1eee8dd9..9604d7b0 100644 > > --- a/test/LuaJIT-tests/common/ffi_util.inc > > +++ b/test/LuaJIT-tests/common/ffi_util.inc > > @@ -4,29 +4,6 @@ > > > > local ffi = require("ffi") > > > > -function checkfail(t, f) > > - f = f or ffi.typeof > > - for i=1,1e9 do > > - local tp = t[i] > > - if not tp then break end > > - assert(pcall(f, tp) == false, tp) > > - end > > -end > > - > > -function checktypes(t) > > - for i=1,1e9,3 do > > - local tp = t[i+2] > > - if not tp then break end > > - local id = ffi.typeof(tp) > > - assert(ffi.sizeof(id) == t[i], tp) > > - assert(ffi.alignof(id) == t[i+1], tp) > > - end > > -end > > - > > -function fails(f, ...) > > - if pcall(f, ...) ~= false then error("failure expected", 2) end > > -end > > - > > local incroot = os.getenv("INCROOT") or "/usr/include" > > local cdefs = os.getenv("CDEFS") or "" > What for these env variables need? They are unused for now, since the function `include()` below is not used anywhere. The first one should be used as a sysroot for finding C includes, and the second one as an additional C defines. > > -- Best regards, Sergey Kaplun