Re: [Tarantool-patches] [PATCH luajit v2] Fix narrowing of unary minus.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

Thanks for the fixes!
LGTM, except a few nits below.

On 20.09.22, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 

<snipped>

>  src/lj_opt_narrow.c                           |  9 +++-
>  .../gh-6976-narrowing-of-unary-minus.test.lua | 51 +++++++++++++++++++
>  2 files changed, 58 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> 
> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
> index cd96ca4b..bb61f97b 100644
> --- a/src/lj_opt_narrow.c
> +++ b/src/lj_opt_narrow.c

<snipped>

> diff --git a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> new file mode 100644
> index 00000000..18778a55
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua
> @@ -0,0 +1,51 @@
> +local tap = require('tap')
> +local test = tap.test('gh-6976-narrowing-of-unary-minus')
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')

As far as we use `table.new` there is no table resizing, so there are no
exits from traces to compile and 'hotexit=1' can be removed.

> +
> +local function check(routine)
> +  jit.off()
> +  jit.flush()
> +  local interp_res = routine()
> +  jit.on()
> +  local jit_res = routine()
> +
> +  for i = 1, #interp_res do
> +    if interp_res[i] ~= jit_res[i] then
> +      return false
> +    end
> +  end
> +
> +  return true
> +end
> +
> +test:ok(

Minor: I suggest the following indentation to make this chunk more
readable:
| test:ok(check(function()
|   local res = require('table.new')(3, 0)
|   for _ = 1, 3 do
|     local zero = 0
|     zero = -zero
|     table.insert(res, tostring(zero))
|   end
|   return res
| end), 'incorrect recording for zero')

Feel free to ignore.

> +  check(
> +    function()
> +      local res = require('table.new')(3, 0)

Please, add a comment why do we use `table.new` here.

> +      for _ = 1, 3 do
> +        local zero = 0
> +        zero = -zero
> +        table.insert(res, tostring(zero))

Minor: we need `tostring()` routine to get different strings for -0 and
0, as far as 0 == -0 as numbers. Please, add the corresponding comment.

> +      end
> +      return res
> +    end
> +  ),
> +  'incorrect recording for zero'
> +)
> +

Ditto, about indentation.
Feel free to ignore.

> +test:ok(
> +  check(
> +    function()
> +      local res = require('table.new')(3, 0)
> +      for i = 2, 0, -1 do
> +        table.insert(res, tostring(-i))
> +      end
> +      return res
> +    end
> +  ),
> +  'assertion guard fail'
> +)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.32.1 (Apple Git-133)
> 

-- 
Best regards,
Sergey Kaplun