Re: [Tarantool-patches] [PATCH luajit] misc: introduce flags with profiler support status

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

Sergey,
Thanks for the fix!
LGTM.

<snipped>

> >>> Don't get the point here:
> >>> 1) We will see this error much earlier in that case.
> >>> 2) We will see with your approach as well.
> >>>
> >>> I don't get the reason for the `type()` call if the check below will
> >>> fail with a different type as well (since Lua checks types first). I see
> >>> no reason in double-checking that means nothing.
> >> The same sense as with checking both boolean value for pcall and an
> >> error message, see for example
> >>
> >> your patch in [1]:
> >>
> >> +test:ok(not result, 'correct status for recursive call')
> >> +test:like(errmsg, 'stack overflow', 'correct error message for
> >> recursive call')
> >>
> >> In aforementioned patch you can check only message, but you check both
> >> values.
> >>
> >> 1.
> >> https://lists.tarantool.org/tarantool-patches/20260316104853.23901-1-skaplun@tarantool.org/T/#u
> > This is not quite the same, IMO. These checks test 2 __different__
> > returned values. 1 -- the status, 2 -- the error message. It is possible
> > that the status is true, and there is no error message as well:
> >
> > | luajit -e 'print(pcall(error)); print(pcall(tonumber, "abc"))'
> > | false   nil
> > | true    nil
> >
> > So, if we get nil, we can distinguish these 2 cases.
> >
> > But from your point of view, the checks should look like the following:
> > |test:ok(type(result) == 'boolean', 'correct status type')
> > |test:ok(not result, 'correct status for recursive call')
> > |test:ok(type(errmsg) == 'string', 'correct errmsg type')
> > |test:like(errmsg, 'stack overflow', 'correct error message for recursive call')
> >
> > Note that in that case, if the type is incorrect, you should modify the
> > test anyway to debug the behaviour (or inspect the test in the GDB).
> >
> > Side note: Anticipating your question: we don't check the type of the
> > first returned value for the `pcall()` because it isn't the unit test
> > for `pcall()`. Hence, we expect that it follows the behaviour declared
> > in the Lua RefMan.
> 
> It's possible that the value's type can change, and it won't be a 
> Boolean value,
> 
> especially since we use a LJLIB_ machinery (imagine you change the "top-2"
> 
> to the "top-3", for example). I want the test to catch this as early as 
> possible.
> 
> It's fair to say that in tests, I have an extreme lack of trust to the 
> system under test, especially when a SUT is a LuaJIT.
> 
> It's easier to parse the results of a failed test there. These checks 
> are cheap
> 
> and only take one line, so I don't understand why we spend so much time 
> discussing this.

I just want to understand your point of view. As I said before -- I
don't insist on the changes.


-- 
Best regards,
Sergey Kaplun