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 CEAAAF30F41; Wed, 12 Feb 2025 10:41:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CEAAAF30F41 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739346119; bh=ZSkX9ZyXTx8bNi2kDJRoV1xh1RYLq6leIf4o+1kTaSo=; 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=aY07Fp+o4j7SLjCbs5MPIp19Vi7IjQdwjeBFavPi+LxZPuWWj3LHpq53XixJ5I+Je yChg/w5y2ES+qNE4JqA2JuLASeE/9xjPb6yqsbj+3yMeTmZKAg5vqxNUyuIP/80a4w sNE8RYtGuMlqs0EvEROaAQnYn5ZDZXrTtXrCHDN0= Received: from send82.i.mail.ru (send82.i.mail.ru [89.221.237.177]) (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 7DA3BF30F41 for ; Wed, 12 Feb 2025 10:41:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7DA3BF30F41 Received: by exim-smtp-79fd7578cb-9g4w9 with esmtpa (envelope-from ) id 1ti7Nd-000000004Je-201P; Wed, 12 Feb 2025 10:41:57 +0300 Date: Wed, 12 Feb 2025 10:41:12 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96946CDE51E80C51A6B03630A98A7F91B1F58DE68326FF884182A05F538085040BE01B4372DCF00953DE06ABAFEAF67055D881F2F90C788F271CCEBC4FC1D6657AAF74BEEA48AC221 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7956F10FFCC7409BAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A0569EA9A35E44F48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D825867FE4EA1FC0DEE06A26E4D2967F50D1C33E19A7310069CC7F00164DA146DAFE8445B8C89999728AA50765F7900637BA939FD1B3BAB99B389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8C2B5EEE3591E0D35F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B08F9A42B2210255C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5E46C029E4025C7705002B1117B3ED6960871A663E27FF5DB22DFD5397F446790823CB91A9FED034534781492E4B8EEAD0AA277257C6A5E3DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34BDCC71B3781C9C9579BFF21AF56501EF6272749F2F09A382954F8363A89CB09A6183A347E2E0216D1D7E09C32AA3244C0C79D3056FF4EA9A77DD89D51EBB7742E924CFC5346FE703EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj/gRw+nncFFCCcedTctH1Fg== X-DA7885C5: F11D85A6267D4B76F255D290C0D534F9368327CEF1FF826B4C3423FBDC110D17C4172ED0C3A24E9D5B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F739381B31377CF4CA219656118AD9C5468A1F29CE480EE0BADD77209B50D1E5063FEE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 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. > > BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it > means Yep, totally 118 warnings for the code we want to check. > > 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. > > > > >> +-- > >> -- [1]:https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/ > >> +-- > >> +-- luacheck: pop > >> > >> -- > >> 2.34.1 > >> -- Best regards, Sergey Kaplun