Hi, Sergey, See my comments below. Sergey On 3/18/26 15:04, Sergey Kaplun wrote: > Sergey, > Thanks for the fixes! > LGTM, after fixing the last comment below. > > On 18.03.26, Sergey Bronnikov wrote: >> Hi, Sergey, >> >> See my answers below. >> >> Sergey >> >> On 3/12/26 20:59, Sergey Kaplun wrote: >>> Hi, Sergey! >>> See my answers below. >>> >>> On 12.03.26, Sergey Bronnikov wrote: >>>> Hi, Sergey, >>>> >>>> thanks for review! See my comments. >>>> >>>> Sergey >>>> >>>> On 3/12/26 15:00, Sergey Kaplun via Tarantool-patches wrote: >>>>> Hi, Sergey! >>>>> Thanks for the patch! >>>>> Please consider my comments below. >>>>> >>>>> On 22.01.26, Sergey Bronnikov wrote: >>>>>> The patch introduce flags in module "misc" with support status >>>>>> for sysprof and memprof: `misc.sysprof.enabled` and >>>>>> `misc.memprof.enabled`. Both flags are boolean and always >>>>> Let's rename it to `available` instead. The `enabled` may be interpreted >>>>> as `is_running`, and confuse the user then. >>>> I propose using `is_available` instead. >>> The Lua API has 0 functions in the snake_case. Even `funcinfo`, >>> `traceexitstub` from lib_jit.c have this naming style. Lets be >>> consistent with it, especially, since `is_` prefix doesn't change the >>> semantics. > > >>>>>> --- >>>>>> src/lib_misc.c | 4 ++++ >>>>>> .../profilers/misclib-memprof-lapi-disabled.test.lua | 5 ++++- >>>>>> test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua | 5 ++++- >>>>>> .../profilers/misclib-sysprof-lapi-disabled.test.lua | 5 ++++- >>>>>> test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 ++++- >>>>>> 5 files changed, 20 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/src/lib_misc.c b/src/lib_misc.c >>>>>> index 034ff878..6b2278c1 100644 >>>>>> --- a/src/lib_misc.c >>>>>> +++ b/src/lib_misc.c >>>>>> @@ -478,7 +478,11 @@ LUALIB_API int luaopen_misc(struct lua_State *L) >>>>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc); >>>>>> #if !LJ_TARGET_WINDOWS >>>>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof); >>>>>> + lua_pushboolean(L, LJ_HASMEMPROF); >>>>>> + lua_setfield(L, -2, "enabled"); >>>>>> LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof); >>>>>> + lua_pushboolean(L, LJ_HASSYSPROF); >>>>>> + lua_setfield(L, -2, "enabled"); >>>>> Is it possible to use standard `LJLIB_PUSH() LJLIB_SET()` machinery >>>>> instead? >>>> I didn't get what is a macros `LJLIB_SET`. >>>> >>>> Also, what are the benefits with using mentioned macros instead more >>>> standard Lua API functions? >>> It is more consistent with the rest of the code base. >>> Also, it helps to avoid table rehasing on library initialization. >>> You may see details in buildvm_lib.c >>> >> Updated: >> >> diff --git a/src/lib_misc.c b/src/lib_misc.c >> index 3463445c..62886242 100644 >> --- a/src/lib_misc.c >> +++ b/src/lib_misc.c >> @@ -389,6 +389,8 @@ LJLIB_CF(misc_sysprof_report) >>  #endif /* !LJ_HASSYSPROF */ >>  } >> >> +LJLIB_PUSH(top-2) LJLIB_SET(available) >> + > Please, add here the: > > |  #include "lj_libdef.h" > > How it is done for the others built-in. It is not crucial for now, but > it helps to be consistent with other modules. Updated: --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -391,6 +391,8 @@ LJLIB_CF(misc_sysprof_report)  LJLIB_PUSH(top-2) LJLIB_SET(available) +#include "lj_libdef.h" +  /* ----- misc.memprof module ---------------------------------------------- */  #define LJLIB_MODULE_misc_memprof > It is necessary to declare one module per time. Otherwise, the sysprof > is declared together with the memprof module. That's not crucial, but > let's be consistent along the codebase to avoid surprises in the future. > >>  /* ----- misc.memprof module >> ---------------------------------------------- */ >> >>  #define LJLIB_MODULE_misc_memprof >> @@ -459,6 +461,8 @@ LJLIB_CF(misc_memprof_stop) >>  } >>  #endif /* !LJ_TARGET_WINDOWS */ >> >> +LJLIB_PUSH(top-2) LJLIB_SET(available) >> + >>  #include "lj_libdef.h" >> >>  /* >> ------------------------------------------------------------------------ */ >> @@ -477,12 +481,10 @@ LUALIB_API int luaopen_misc(struct lua_State *L) >> >>    LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc); >>  #if !LJ_TARGET_WINDOWS >> -  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof); >>    lua_pushboolean(L, LJ_HASMEMPROF); >> -  lua_setfield(L, -2, "available"); >> -  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof); >> +  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof); >>    lua_pushboolean(L, LJ_HASSYSPROF); >> -  lua_setfield(L, -2, "available"); >> +  LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof); >>  #endif /* !LJ_TARGET_WINDOWS */ >>    return 1; >>  } >> >>>>>> #endif /* !LJ_TARGET_WINDOWS */ >>>>>> return 1; >>>>>> } >>>>>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua >>>>>> index de0aa136..f867cfc6 100644 >>>>>> --- a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua >>>>>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua > > >>>>>> >>>>>> +test:ok(type(misc.memprof.enabled) == 'boolean', 'misc.memprof.enabled exists') >>>>> I suppose that >>>>> |test:is(misc.memprof.available, false, 'misc.memprof.enabled correct') >>>>> is enough. >>>>> >>>>> Same for other tests below. >>>> if "misc.memprof" is not a table the error will be "attempt to index field" >>> 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 don't insist on these changes but still don't understand your point, > though. > >>> Also, `is()` check is more verbose in case of failure -- it prints got >>> and expected values. >> It's much easier for me to read that a type check failed than to figure >> out why the value is incorrect. >> >> The TAP checks produce completely unreadable message; I never use them. > I use and it may be helpful. I don't insisth, though. > >>>>>> +test:ok(misc.memprof.enabled == false, 'misc.memprof.enabled is false') >>>>>> + > >