Re: [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib*

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.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