Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib*
Date: Sun, 14 Feb 2021 22:16:26 +0300
Message-ID: <20210214191626.GF9361@root> (raw)
In-Reply-To: <2db1d3a53682de0bb4231ec494fc44e95acd9a8f.1612291495.git.imun@tarantool.org>

Hi, Igor!

Thanks for the patch!

Looks like I was late for the ball and you got two LGTMs, but here’s my
two cents on the matter, because I've been mentioned. Feel free to
ignore my nitpicks.

LGTM, except two nitpicks below.

On 02.02.21, Igor Munkin wrote:
> Since the regular static analysis has not been enabled for the test
> chunks in LuaJIT repository yet, the tests for recently implemented
> features still produce luacheck warnings.
> 
> The most of the issues are fixed in scope of the commit
> 8fc103fb1a21c28185a1942e75d8d9485e3aade7 ('test: fix warnings spotted by
> luacheck') and this patch fixes the remaining ones.
> 
> Fixes tarantool/tarantool#5631

Nit: Looks like 'Closes' or even 'Part of' (considering the next patch),
not 'Fixes'.
Also, After this patch there are tons of misc-related warnings
by the luacheсk without config. So, I think that this patch should be
squashed with the next one.
Feel free to ignore.

> Part of tarantool/tarantool#4862
> Part of tarantool/tarantool#5470
> Follows up tarantool/tarantool#5187
> 
> Reported-by: Sergey Bronnikov <sergeyb@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  .../misclib-getmetrics-capi.test.lua          | 22 ++++++---------
>  .../misclib-getmetrics-lapi.test.lua          | 28 +++++++++----------
>  2 files changed, 23 insertions(+), 27 deletions(-)
> 
> diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> index c418e9f..d409ea9 100755
> --- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> @@ -38,29 +38,29 @@ test:ok(testgetmetrics.objcount(function(iterations)
>      }
>  
>      -- Separate objects creations to separate jit traces.
> -    for i = 1, iterations do
> -        table.insert(placeholder.str, tostring(i))
> +    for _ = 1, iterations do
> +        table.insert(placeholder.str, tostring(_))
>      end
>  
> -    for i = 1, iterations do
> -        table.insert(placeholder.tab, {i})
> +    for _ = 1, iterations do
> +        table.insert(placeholder.tab, {_})
>      end
>  
> -    for i = 1, iterations do
> +    for _ = 1, iterations do
>          table.insert(placeholder.udata, newproxy())
>      end
>  
> -    for i = 1, iterations do
> +    for _ = 1, iterations do
>          -- Check counting of VLA/VLS/aligned cdata.
>          table.insert(placeholder.cdata, ffi.new("char[?]", 4))
>      end
>  
> -    for i = 1, iterations do
> +    for _ = 1, iterations do

Side note: this change is not necessary for the patch, but the code looks
more consistent. So, it is good, thanks!

>          -- Check counting of non-VLA/VLS/aligned cdata.
> -        table.insert(placeholder.cdata, ffi.new("uint64_t", i))
> +        table.insert(placeholder.cdata, ffi.new("uint64_t", _))
>      end
>  

<snipped>

> diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> index 959293d..d54caac 100755
> --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> @@ -58,10 +58,10 @@ test:test("gc-allocated-freed", function(subtest)
>      until collectgarbage("count") == count
>  
>      -- Bump getmetrics table and string keys allocation.
> -    local old_metrics = misc.getmetrics()
> +    misc.getmetrics()

Side note: This is a very elegant solution! Cool!

>  
>      -- Remember allocated size for getmetrics table.
> -    old_metrics = misc.getmetrics()
> +    local old_metrics = misc.getmetrics()

<snipped>

> @@ -372,7 +372,7 @@ test:test("strhash", function(subtest)
>      assert(new_metrics.strhash_miss - old_metrics.strhash_miss == 0)
>      old_metrics = new_metrics
>  
> -    local str1  = "strhash".."_hit"
> +    local _ = "strhash".."_hit"
>  
>      new_metrics = misc.getmetrics()
>      assert(new_metrics.strhash_hit - old_metrics.strhash_hit == 20)
> @@ -384,7 +384,7 @@ test:test("strhash", function(subtest)
>      assert(new_metrics.strhash_miss - old_metrics.strhash_miss == 0)
>      old_metrics = new_metrics
>  
> -    local str2 = "new".."string"
> +    local _ = "new".."string"

Nit: this `local` is redundant.
Feel free to ignore.

>  
>      new_metrics = misc.getmetrics()
>      assert(new_metrics.strhash_hit - old_metrics.strhash_hit == 19)
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

  parent reply	other threads:[~2021-02-14 19:17 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-02 20:57 [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 1/5] build: preserve the original build system Igor Munkin via Tarantool-patches
2021-02-04 22:53   ` Timur Safin via Tarantool-patches
2021-02-08 15:56     ` Igor Munkin via Tarantool-patches
2021-02-09 11:38   ` Sergey Kaplun via Tarantool-patches
2021-02-09 12:47     ` Igor Munkin via Tarantool-patches
2021-02-09 14:45       ` Sergey Kaplun via Tarantool-patches
2021-02-09 15:28         ` Igor Munkin via Tarantool-patches
2021-02-10  9:35           ` Sergey Kaplun via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake Igor Munkin via Tarantool-patches
2021-02-04 22:53   ` Timur Safin via Tarantool-patches
2021-02-08 15:56     ` Igor Munkin via Tarantool-patches
2021-02-09 13:55       ` Timur Safin via Tarantool-patches
2021-02-09 15:09         ` Igor Munkin via Tarantool-patches
2021-02-11 19:23   ` Sergey Kaplun via Tarantool-patches
2021-02-16 15:28     ` Igor Munkin via Tarantool-patches
2021-02-18  9:56       ` Sergey Kaplun via Tarantool-patches
2021-02-20 19:18         ` Igor Munkin via Tarantool-patches
2021-02-27 10:48           ` Sergey Kaplun via Tarantool-patches
2021-02-28 18:18             ` Igor Munkin via Tarantool-patches
2021-02-13  3:47   ` Sergey Kaplun via Tarantool-patches
2021-02-16 15:32     ` Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake Igor Munkin via Tarantool-patches
2021-02-08 15:05   ` Timur Safin via Tarantool-patches
2021-02-08 16:29     ` Igor Munkin via Tarantool-patches
2021-02-09  8:16       ` Timur Safin via Tarantool-patches
2021-02-09  8:43         ` Igor Munkin via Tarantool-patches
2021-02-09 13:59           ` Timur Safin via Tarantool-patches
2021-02-09 15:10             ` Igor Munkin via Tarantool-patches
2021-02-14 18:48   ` Sergey Kaplun via Tarantool-patches
2021-02-19 19:04     ` Igor Munkin via Tarantool-patches
2021-02-27 13:50       ` Sergey Kaplun via Tarantool-patches
2021-02-28 18:18         ` Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib* Igor Munkin via Tarantool-patches
     [not found]   ` <012f01d6fe1a$a2aa6890$e7ff39b0$@tarantool.org>
     [not found]     ` <2c495492-50f4-acfd-ad66-2cb44abb5fa1@tarantool.org>
2021-02-08 15:40       ` Sergey Bronnikov via Tarantool-patches
2021-02-08 15:58       ` Igor Munkin via Tarantool-patches
2021-02-08 15:57     ` Igor Munkin via Tarantool-patches
2021-02-14 19:16   ` Sergey Kaplun via Tarantool-patches [this message]
2021-02-16 15:29     ` Igor Munkin via Tarantool-patches
2021-02-16 16:36       ` Sergey Kaplun via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 5/5] test: run luacheck static analysis via CMake Igor Munkin via Tarantool-patches
2021-02-04 22:52   ` Timur Safin via Tarantool-patches
2021-02-08 15:57     ` Igor Munkin via Tarantool-patches
2021-02-14 19:32   ` Sergey Kaplun via Tarantool-patches
2021-02-19 19:14     ` Igor Munkin via Tarantool-patches
2021-02-28 22:04 ` [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210214191626.GF9361@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git