Re: [Tarantool-patches] [PATCH luajit] test: limit code and comment max length by 80

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

[-- Attachment #1: Type: text/plain, Size: 13085 bytes --]

Hi, Sergey

thanks for review! Please see my comments.

On 11.02.2025 14:51, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> It's a nice idea to automate this part of the review!
>
> Please consider my questions below.
>
> On 11.02.25, Sergey Bronnikov wrote:
>> The patch sets a max length with 80 symbols and fixes tests
>> exceeding this length.
>> ---
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-set-max-length
>> Luacheck configuration file:
>> https://luacheck.readthedocs.io/en/stable/config.html
>>
>>   .luacheckrc                                            |  5 +++++
>>   test/tarantool-tests/fix-argv-handling.test.lua        |  4 ++++
>>   .../gh-3196-incorrect-string-length.test.lua           |  3 +++
>>   test/tarantool-tests/gh-6163-min-max.test.lua          |  4 ++++
>>   .../lj-366-strtab-correct-size.test.lua                | 10 ++++++++--
>>   .../lj-494-table-chain-infinite-loop.test.lua          |  3 ++-
>>   test/tarantool-tests/lj-688-snap-ir-rename.test.lua    |  2 ++
>>   test/tarantool-tests/lj-819-fix-missing-uclo.test.lua  |  6 ++++--
>>   .../lj-918-fma-numerical-accuracy-jit.test.lua         |  4 ++++
>>   .../lj-918-fma-numerical-accuracy.test.lua             |  4 ++++
>>   test/tarantool-tests/or-144-gc64-asmref-l.test.lua     |  4 ++++
>>   11 files changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/.luacheckrc b/.luacheckrc
>> index f2573e42..8047677c 100644
>> --- a/.luacheckrc
>> +++ b/.luacheckrc
>> @@ -3,6 +3,11 @@ std = 'luajit'
>>   -- This fork also introduces a new global for misc API namespace.
>>   read_globals = { 'misc' }
>>   
>> +max_line_length = 80
> I suppose this is excess since we have all other settings intact.
Removed.
>
>> +max_code_line_length = 80
>> +max_string_line_length = 80
> Do we need this particular check? Maybe it is covered by the
> `max_code_line_length` and `max_line_length`. Can we avoid markers in
> <gh-3196-incorrect-string-length.test.lua> without it?
>
>> +max_comment_line_length = 80
> Actually, the comment length for the Lua part is 66 symbols. Maybe we
> may omit this check to avoid stubs in the comments?

The main idea is automating this check and avoid comments like

"your comment is exceeded XXX symbols" on review. So I don't understand

how to omit this check.

BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it 
means

that manual (visual) checking of max length in comments is unreliable 
and we definitely

should automate it.

>
>> +
>>   -- The `_TARANTOOL` global is often used for skip condition
>>   -- checks in tests.
>>   files['test/tarantool-tests/'] = {
>> diff --git a/test/tarantool-tests/fix-argv-handling.test.lua b/test/tarantool-tests/fix-argv-handling.test.lua
>> index 84b626c3..5f147197 100644
>> --- a/test/tarantool-tests/fix-argv-handling.test.lua
>> +++ b/test/tarantool-tests/fix-argv-handling.test.lua
>> @@ -10,7 +10,11 @@ test:plan(1)
>>   -- to a single empty string if it is empty [1], so the issue is
>>   -- not reproducible on new kernels.
>>   --
>> +-- luacheck: push no max comment line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global

AFAIR, no. Global suppression will cover the whole file, I don't think

it is a good idea. Approach with push/pop was already used in

commit 9ac79bd7cd12dc3dbfa0e502e29a99b3083676c1
  ("test: introduce test:done TAP helper") why we cannot continue using 
this approach for other files?

>
>> +--
>>   -- [1]:https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/
>> +--
>> +-- luacheck: pop
>>   
>>   local utils = require('utils')
>>   local execlib = require('execlib')
>> diff --git a/test/tarantool-tests/gh-3196-incorrect-string-length.test.lua b/test/tarantool-tests/gh-3196-incorrect-string-length.test.lua
>> index b82029f6..79521608 100644
>> --- a/test/tarantool-tests/gh-3196-incorrect-string-length.test.lua
>> +++ b/test/tarantool-tests/gh-3196-incorrect-string-length.test.lua
>> @@ -6,10 +6,13 @@ test:plan(2)
>>   --
>>   -- gh-3196: incorrect string length if Lua hash returns 0
>>   --
>> +-- luacheck: push no max code line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global
See above.
>> +--
>>   local h = "\x1F\x93\xE2\x1C\xCA\xDE\x28\x08\x26\x01\xED\x0A\x2F\xE4\x21\x02\x97\x77\xD9\x3E"
>>   test:is(h:len(), 20)
>>   
>>   h = "\x0F\x93\xE2\x1C\xCA\xDE\x28\x08\x26\x01\xED\x0A\x2F\xE4\x21\x02\x97\x77\xD9\x3E"
>>   test:is(h:len(), 20)
>> +-- luacheck: pop
>>   
>>   test:done(true)
>> diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
>> index e3efd1a4..ca6c2a7f 100644
>> --- a/test/tarantool-tests/gh-6163-min-max.test.lua
>> +++ b/test/tarantool-tests/gh-6163-min-max.test.lua
>> @@ -69,9 +69,13 @@ jit.opt.start('hotloop=1')
>>   -- pair is emitted, and the result is `NaN`, which is inconsistent with
>>   -- the result of the non-optimized mcode.
>>   --
>> +-- luacheck: push no max comment line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global
>
>> +--
>>   -- [1]:https://developer.arm.com/documentation/dui0801/g/A64-Floating-point-Instructions/FCMP
>>   -- [2]:https://developer.arm.com/documentation/100069/0608/A64-Floating-point-Instructions/FCSEL
>>   -- [3]:https://developer.arm.com/documentation/dui0068/b/ARM-Instruction-Reference/Conditional-execution
>> +--
>> +-- luacheck: pop
>>   
>>   local result = {}
>>   for k = 1, 4 do
>> 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 a20339e5..b46ace49 100644
>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> @@ -22,11 +22,15 @@ local utils = require('utils')
>>   --
>>   -- $ readelf --symbols xxx.obj
>>   --
>> +-- luacheck: push no max comment line length
>> +--
>>   -- Symbol table '.symtab' contains 2 entries:
>>   --    Num:    Value          Size Type    Bind   Vis      Ndx Name
>>   --      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>>   --      1: 0000000000000000    66 OBJECT  GLOBAL DEFAULT    4 luaJIT_BC_lango_team
>>   --
>> +-- luacheck: pop
>> +--
>>   -- and to display the information contained in the file's section
>>   -- headers, if it has any. For our purposes, we are interested in
>>   -- .symtab section, so other sections are snipped in the output:
>> @@ -143,7 +147,8 @@ end
>>   -- and symtab sections.
>>   local function read_elf(elf_content)
>>     local ELFobj_type = ffi.typeof(is64 and 'ELF64obj *' or 'ELF32obj *')
>> -  local ELFsectheader_type = ffi.typeof(is64 and 'ELF64sectheader *' or 'ELF32sectheader *')
>> +  local ELFsectheader_type = ffi.typeof(is64 and 'ELF64sectheader *' or
>> +                                        'ELF32sectheader *')
>>     local elf = ffi.cast(ELFobj_type, elf_content)
>>     local symtab_hdr, strtab_hdr
>>     -- Iterate by section headers.
>> @@ -178,7 +183,8 @@ assert(sym_cnt ~= 0, 'number of symbols is zero')
>>   test:is(strtab_size, EXPECTED_STRTAB_SIZE, 'check .strtab size')
>>   test:is(strtab_offset, EXPECTED_STRTAB_OFFSET, 'check .strtab offset')
>>   
>> -local strtab_str = string.sub(elf_content, strtab_offset, strtab_offset + strtab_size)
>> +local strtab_str = string.sub(elf_content, strtab_offset,
>> +                              strtab_offset + strtab_size)
>>   
>>   local strtab_p = ffi.cast('char *', strtab_str)
>>   local sym_is_found = false
>> diff --git a/test/tarantool-tests/lj-494-table-chain-infinite-loop.test.lua b/test/tarantool-tests/lj-494-table-chain-infinite-loop.test.lua
>> index 3dd17e7a..1c1d7128 100644
>> --- a/test/tarantool-tests/lj-494-table-chain-infinite-loop.test.lua
>> +++ b/test/tarantool-tests/lj-494-table-chain-infinite-loop.test.lua
>> @@ -172,6 +172,7 @@ collectgarbage()
>>   for c, v in pairs(victims) do
>>       str_hash(v)[0] = orig_hash[c]
>>   end
>> -test:ok(true, "table keys collisions are resolved properly (no assertions failed)")
>> +test:ok(true,
>> +        "table keys collisions are resolved properly (no assertions failed)")
>>   
>>   test:done(true)
>> diff --git a/test/tarantool-tests/lj-688-snap-ir-rename.test.lua b/test/tarantool-tests/lj-688-snap-ir-rename.test.lua
>> index 807e0811..5b458cd0 100644
>> --- a/test/tarantool-tests/lj-688-snap-ir-rename.test.lua
>> +++ b/test/tarantool-tests/lj-688-snap-ir-rename.test.lua
>> @@ -41,7 +41,9 @@ jit.opt.start('hotloop=1')
>>   -- platform-agnostic, there is no real necessity to test it
>>   -- against ARM/ARM64 separately.
>>   --
>> +-- luacheck: push no max comment line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global
>
>>   -- See alsohttps://drive.google.com/file/d/1iYkFx3F0DOtB9o9ykWfCEm-OdlJNCCL0/view?usp=share_link
>> +-- luacheck: pop
>>   
>>   
>>   local vals = {-0.1, 0.1, -0.1, 0.1}
>> diff --git a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> index e2352c92..43a65eb8 100644
>> --- a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> @@ -100,7 +100,8 @@ local f = missing_uclo()
>>   local res = f()
>>   -- Without a patch we don't get here a function, because upvalue isn't closed
>>   -- as desirable.
>> -test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct')
>> +test:ok(type(res) == 'function',
>> +        'virtual machine consistency: type of returned value is correct')
>>   
>>   -- Make JIT compiler aggressive.
>>   jit.opt.start('hotloop=1')
>> @@ -110,6 +111,7 @@ f()
>>   f = missing_uclo()
>>   local _
>>   _, res = pcall(f)
>> -test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct')
>> +test:ok(type(res) == 'function',
>> +        'consistency on compilation: type of returned value is correct')
>>   
>>   test:done(true)
>> diff --git a/test/tarantool-tests/lj-918-fma-numerical-accuracy-jit.test.lua b/test/tarantool-tests/lj-918-fma-numerical-accuracy-jit.test.lua
>> index 8b16d4c3..43c1a4a9 100644
>> --- a/test/tarantool-tests/lj-918-fma-numerical-accuracy-jit.test.lua
>> +++ b/test/tarantool-tests/lj-918-fma-numerical-accuracy-jit.test.lua
>> @@ -22,9 +22,13 @@ local _2pow52 = 2 ^ 52
>>   -- double rounding. The numbers from the original issue are good
>>   -- enough.
>>   --
>> +-- luacheck: push no max comment line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global
>
>> +--
>>   -- [1]:https://developer.arm.com/documentation/dui0801/g/A64-Floating-point-Instructions/FMSUB
>>   -- [2]:https://en.wikipedia.org/wiki/Multiply%E2%80%93accumulate_operation
>>   --
>> +-- luacheck: pop
>> +--
>>   -- IEEE754 components to double:
>>   -- sign * (2 ^ (exp - 1023)) * (mantissa / _2pow52 + normal).
>>   local a = 1 * (2 ^ (1083 - 1023)) * (4080546448249347 / _2pow52 + 1)
>> diff --git a/test/tarantool-tests/lj-918-fma-numerical-accuracy.test.lua b/test/tarantool-tests/lj-918-fma-numerical-accuracy.test.lua
>> index 25b59707..2472f05a 100644
>> --- a/test/tarantool-tests/lj-918-fma-numerical-accuracy.test.lua
>> +++ b/test/tarantool-tests/lj-918-fma-numerical-accuracy.test.lua
>> @@ -20,9 +20,13 @@ local _2pow52 = 2 ^ 52
>>   -- double rounding. The numbers from the original issue are good
>>   -- enough.
>>   --
>> +-- luacheck: push no max comment line length
> Side note: Can it will be done without push/pop interface? Like for
> globals we use:
> | luacheck: no global
>
>> +--
>>   -- [1]:https://developer.arm.com/documentation/dui0801/g/A64-Floating-point-Instructions/FMSUB
>>   -- [2]:https://en.wikipedia.org/wiki/Multiply%E2%80%93accumulate_operation
>>   --
>> +-- luacheck: pop
>> +--
>>   -- IEEE754 components to double:
>>   -- sign * (2 ^ (exp - 1023)) * (mantissa / _2pow52 + normal).
>>   local a = 1 * (2 ^ (1083 - 1023)) * (4080546448249347 / _2pow52 + 1)
>> diff --git a/test/tarantool-tests/or-144-gc64-asmref-l.test.lua b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua
>> index 18c6efb2..cbc13278 100644
>> --- a/test/tarantool-tests/or-144-gc64-asmref-l.test.lua
>> +++ b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua
>> @@ -5,11 +5,15 @@ local test = tap.test('or-144-gc64-asmref-l'):skipcond({
>>   
>>   test:plan(1)
>>   
>> +-- luacheck: push no max comment line length
>> +--
>>   -- Test file to demonstrate LuaJIT `IR_LREF` assembling incorrect
>>   -- behaviour.
>>   -- See also:
>>   -- *https://github.com/openresty/lua-resty-core/issues/144.
>>   -- *https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode.
>> +--
>> +-- luacheck: pop
>>   
>>   jit.opt.start('hotloop=1')
>>   
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 16414 bytes --]