From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <estetus@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
Date: Mon, 8 Apr 2024 10:47:00 +0300 [thread overview]
Message-ID: <ZhOg9MFA4nKg2EP-@root> (raw)
In-Reply-To: <cover.1710416150.git.sergeyb@tarantool.org>
Hi, Sergey!
Thanks for the patch set!
I'll proceed with the review according version on the branch:
===================================================================
> Subject: [PATCH 1/3] test: introduce a helper read_file
This patch LGTM, after fixing several nits below.
>
> The test `lj-366-strtab-correct-size.test.lua` has a test helper
> `read_file` that reads a file's content and returns it.
> This helper will be useful for a test upcoming in the next commit,
> so it is moved to test tools.
So, missed the ticket mentioning:
| Needed for tarantool/tarantool#9595
> ---
> .../lj-366-strtab-correct-size.test.lua | 10 +---------
> test/tarantool-tests/utils/tools.lua | 8 ++++++++
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> index 8a97a441..0bb92da6 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> @@ -138,14 +138,6 @@ local function create_obj_file(name)
> return elf_filename
> end
>
> --- Reads a file located in a specified path and returns its content.
> -local function read_file(path)
> - local file = assert(io.open(path), 'cannot open an object file')
> - local content = file:read('*a')
> - file:close()
> - return content
> -end
> -
> -- Parses a buffer in an ELF format and returns an offset and a size of strtab
> -- and symtab sections.
> local function read_elf(elf_content)
> @@ -172,7 +164,7 @@ end
> test:plan(3)
>
> local elf_filename = create_obj_file(MODULE_NAME)
> -local elf_content = read_file(elf_filename)
> +local elf_content = require('utils').tools.read_file(elf_filename)
Minor: I suggest avoiding the change at this line.
According to codestyle in other tests, we require helpers separately,
even if they are used only once.
> assert(#elf_content ~= 0, 'cannot read an object file')
>
> local strtab, symtab = read_elf(elf_content)
> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
> index f35c6922..26b8c08d 100644
> --- a/test/tarantool-tests/utils/tools.lua
> +++ b/test/tarantool-tests/utils/tools.lua
> @@ -12,4 +12,12 @@ function M.profilename(name)
> return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
> end
>
> +-- Reads a file located in a specified path and returns its content.
Typo: s/in/at/
> +function M.read_file(path)
> + local file = assert(io.open(path), 'cannot open an object file')
> + local content = file:read('*a')
> + file:close()
> + return content
> +end
> +
> return M
> --
> 2.44.0
===================================================================
--
Best regards,
Sergey Kaplun
next prev parent reply other threads:[~2024-04-08 7:51 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 11:39 Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches
2024-04-09 12:47 ` Sergey Kaplun via Tarantool-patches
2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:45 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
2024-04-11 7:56 ` Sergey Bronnikov via Tarantool-patches
2024-04-08 7:47 ` Sergey Kaplun via Tarantool-patches [this message]
2024-04-08 13:06 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches
2024-04-11 12:39 ` Sergey Bronnikov via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZhOg9MFA4nKg2EP-@root \
--to=tarantool-patches@dev.tarantool.org \
--cc=estetus@gmail.com \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox