Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi,

On 11.04.24, Sergey Bronnikov wrote:
> Hi,
> 
> On 08.04.2024 10:47, Sergey Kaplun wrote:
> > 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
> It is actually not needed for 9595.

OK, not insisting.

> >
> >> ---
> >>   .../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.
> Where can I read a codestyle guide which you want to follow?

This is just a general tradition for all test files. Usually, it is a
good thing to do it this way because we don't want to use a lookup for
the module every time the function is called. This is not related to
your case directly, but if somebody moves this code to the function, he
should also change this part anyway. Plus, as a bonus, we will get an
earlier failure if the `read_file` module doesn't exist.

> >
> >>   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/
> 
> Everything fine from Quillbot point of view.

But I have it, see [1].
Sometimes it freezes, so you should reload the current page.

> 
> 
> >
> >> +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
> > ===================================================================
> >

[1]: https://ibb.co/XzCPC13

-- 
Best regards,
Sergey Kaplun