Hi, Sergey
thanks for review! Please see my comments.
Removed.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 = 80I suppose this is excess since we have all other settings intact.
+max_code_line_length = 80 +max_string_line_length = 80Do 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 = 80Actually, 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 lengthSide 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?
See above.+-- -- [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 lengthSide note: Can it will be done without push/pop interface? Like for globals we use: | luacheck: no global
+-- 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 lengthSide 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 lengthSide note: Can it will be done without push/pop interface? Like for globals we use: | luacheck: no global-- See also https://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 lengthSide 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 lengthSide 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