<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey!<br>
</p>
<div class="moz-cite-prefix">On 12.02.2025 10:41, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z6xQmL0-IW_4VRl4@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the clarification.
See my answers below.
On 12.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hi, Sergey
thanks for review! Please see my comments.
On 11.02.2025 14:51, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">The patch sets a max length with 80 symbols and fixes tests
exceeding this length.
---
Branch:<a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-set-max-length">https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-set-max-length</a>
Luacheck configuration file:
<a class="moz-txt-link-freetext" href="https://luacheck.readthedocs.io/en/stable/config.html">https://luacheck.readthedocs.io/en/stable/config.html</a>
.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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I suppose this is excess since we have all other settings intact.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Removed.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+max_code_line_length = 80
+max_string_line_length = 80
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+max_comment_line_length = 80
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Actually, the comment length for the Lua part is 66 symbols. Maybe we
may omit this check to avoid stubs in the comments?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Sorry, I misled you with bad wording.
Originally, according to the commit message. I think that we would
change the limit to 80 symbols for all code (and check comments manually
or don't check them at all). But since we want to do it fully
automated,
we should use the following 2 lines:
| max_code_line_length = 80
| max_comment_line_length = 66</pre>
</blockquote>
Well, did it in updated patch.<br>
<blockquote type="cite" cite="mid:Z6xQmL0-IW_4VRl4@root">
<pre class="moz-quote-pre" wrap="">
Or we still should check manually the comment line length on the review.
We don't need `max_line_length`, since we distinguish lines with/without
comments.
And we don't need string lines, I suppose. At least I don't fully
understand why we need them.</pre>
</blockquote>
both removed from config.<br>
<blockquote type="cite" cite="mid:Z6xQmL0-IW_4VRl4@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it
means
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yep, totally 118 warnings for the code we want to check.</pre>
</blockquote>
<p>All warnings were fixed and patch force-pushed to the branch.</p>
<p>See v2 version in ML.<br>
</p>
<blockquote type="cite" cite="mid:Z6xQmL0-IW_4VRl4@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
that manual (visual) checking of max length in comments is unreliable
and we definitely
should automate it.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Totally agree.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
-- 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Side note: Can it will be done without push/pop interface? Like for
globals we use:
| luacheck: no global
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'm not against it. I just prefer the inline suppressions.
Actually we _may_ use
| -- luacheck: no max_comment_line_length
But this is a global (the whole file below this line) suppression.
I thought that this suppresses warnings until the next line of code, like
ignore does. BTW,
| -- luacheck: ignore
does exactly what I want, but unfortunately it may not be clear what
exactly we ignore :).
But this is a matter of taste, so feel free to choose any option here.
</pre>
</blockquote>
<p>Discussed it in a private conversation and decided a variant with
push/pop</p>
<p>and a warning name with underscores instead whitespaces.</p>
<blockquote type="cite" cite="mid:Z6xQmL0-IW_4VRl4@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+--
-- [1]:<a class="moz-txt-link-freetext" href="https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/">https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/</a>
+--
+-- luacheck: pop
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">--
2.34.1
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>