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

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

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.
>
>> ---
>>   .../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?
>
>>   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.


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