From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 92E50F3A740; Wed, 12 Feb 2025 16:35:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 92E50F3A740 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739367307; bh=0HFp0ksFmKCIrwrnrrif5Z1jTjWxkB+OatjYhzTr35s=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=fF1IjjGdXHKZ690nlQ9BGaL/ndKezI5RaYw0n1CXWGku8dY/32T4oYvBBNVkG57QD /R2ZHNnv48Z4ddmM0ZBbGyjlEP/qO+Aj8wxw4KtYp7PL+T9tpv6BndbNewElF5/3BY hB1656+rbb9hwOjrLjEIvPPtT0LTHHlkzbvRKGLc= Received: from send103.i.mail.ru (send103.i.mail.ru [89.221.237.198]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EF668F3F940 for ; Wed, 12 Feb 2025 16:35:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EF668F3F940 Received: by exim-smtp-79fd7578cb-4t99f with esmtpa (envelope-from ) id 1tiCtN-00000000IED-2wSG; Wed, 12 Feb 2025 16:35:06 +0300 Content-Type: multipart/alternative; boundary="------------60XZTAJZUPu8D8XAXxQVtx96" Message-ID: <2f028e6e-69e5-4930-bb62-bc8771e33355@tarantool.org> Date: Wed, 12 Feb 2025 16:35:05 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org References: In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD93899CACFFF273C59D44F46A6FADFF312BEDF55125960AD11182A05F538085040B4B9EA559E8A40383DE06ABAFEAF6705CE9EA29FD6BAE3BBBFE19D66EE800B2A95F83A364D46ACBB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE727FD6E7FC3A8F857EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063748744D4CD6EC491D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88B0AC73654B22E91BAB8E81D721CB347F726839D81F6314DCC7F00164DA146DAFE8445B8C89999728AA50765F79006370BDB19F53EE528DD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8744B801E316CB65FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C4E7D9683544204AF9735652A29929C6C4AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3F6FBD7DDCBC7A9A5BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE71D0063F52110EA4A731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A57F30E43041F2951E5002B1117B3ED69669C76DA5ED3EB104219207EC0A953D2C823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D340EEB18E491831571C798582CC7DCEA71C3A7DBF5630437F72DC8BA7ADD9F15B3DF330D0F4243CE3D1D7E09C32AA3244C914D5E7EA291FC6377DD89D51EBB7742A81941489D5A4C4FEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj/gRw+nncFFClnT9E4LbVhg== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61405150B7B3D035F8B0C7D0F6C13AE992130DD3458B398D82750152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] test: limit code and comment max length by 80 X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------60XZTAJZUPu8D8XAXxQVtx96 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey! On 12.02.2025 10:41, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the clarification. > See my answers below. > > On 12.02.25, Sergey Bronnikov wrote: >> 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 >>> 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. > 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 Well, did it in updated patch. > > 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. both removed from config. > >> BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it >> means > Yep, totally 118 warnings for the code we want to check. All warnings were fixed and patch force-pushed to the branch. See v2 version in ML. > >> that manual (visual) checking of max length in comments is unreliable >> and we definitely >> should automate it. > Totally agree. > >>>> + >>>> -- 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? > 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. > Discussed it in a private conversation and decided a variant with push/pop and a warning name with underscores instead whitespaces. >>>> +-- >>>> -- [1]:https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/ >>>> +-- >>>> +-- luacheck: pop >>>> > > >>>> -- >>>> 2.34.1 >>>> --------------60XZTAJZUPu8D8XAXxQVtx96 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey!

On 12.02.2025 10:41, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the clarification.
See my answers below.

On 12.02.25, Sergey Bronnikov wrote:

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.


      

      
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

    

    Well, did it in updated patch.

    

      

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.

    

    both removed from config.

    

      



      

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


      

      
Yep, totally 118 warnings for the code we want to check.

    

    
All warnings were fixed and patch force-pushed to the branch.

    
See v2 version in ML.

    

    

      



      

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


      

      
Totally agree.



      

        

        

          

          

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


      

      
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.



    

    
Discussed it in a private conversation and decided a variant with
      push/pop

    
and a warning name with underscores instead whitespaces.

    

      

        

        

          

          

            
+--
  -- [1]:https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/
+--
+-- luacheck: pop
  


          

        

      

      
<snipped>



      

        

          

            
-- 
2.34.1



          

        

      

      

    

  
  


--------------60XZTAJZUPu8D8XAXxQVtx96--