Re: [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for platform metrics

[Sergey Kaplun <skaplun@tarantool.org>]

Thanks, again! Great comments as always!

On 07.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the fixes! The patch LGTM, except two nits below.
> 
> On 07.10.20, Sergey Kaplun wrote:
> > Hi Igor! Thanks for the review!
> > 
> > On 07.10.20, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > Thanks for the patch! Please consider my comments below.
> > > 

<snipped>

> > diff --git a/test/misclib-getmetrics-capi.test.lua b/test/misclib-getmetrics-capi.test.lua
> > index 34adaba..f133274 100755
> > --- a/test/misclib-getmetrics-capi.test.lua
> > +++ b/test/misclib-getmetrics-capi.test.lua
> > @@ -1,15 +1,15 @@
> >  #!/usr/bin/env tarantool
> >  
> > -local file = debug.getinfo(1, "S").source:sub(2)
> > -local filepath = file:match("(.*/)")
> > -local soext = jit.os == "OSX" and "dylib" or "so"
> > -package.cpath = filepath..'clib-misclib-getmetrics/?.'..soext..";"
> > -                ..package.cpath
> > +local path = arg[0]:gsub('%.test%.lua', '')
> > +local suffix = package.cpath:match('?.(%a+);')
> > +package.cpath = ('%s/?.%s'):format(path, suffix)
> 
> This line overrides the current <package.cpath>.

Sure! 10x!

> 
> >  
> 
> <snipped>
> 
> > diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> > index 32802d2..4a7355a 100644
> > --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> > +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> 
> <snipped>
> 
> > @@ -203,7 +207,6 @@ static int tracenum_base(lua_State *L)
> >  	luaM_metrics(L, &metrics);
> >  	assert(metrics.jit_trace_num == 0);
> >  
> > -	luaJIT_setmode(L, 0, LUAJIT_MODE_ON);
> 
> There is one more excess call at the end of this function.

Thanks! Nice catch!

> 
> >  	/* Generate traces. */
> >  	lua_call(L, 0, 1);
> >  	n = lua_gettop(L);
> 
> <snipped>
> 
> > ===================================================================
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

See iterative patch in the bottom. Branch force-pushed.

===================================================================
diff --git a/test/misclib-getmetrics-capi.test.lua b/test/misclib-getmetrics-capi.test.lua
index f133274..1ad6958 100755
--- a/test/misclib-getmetrics-capi.test.lua
+++ b/test/misclib-getmetrics-capi.test.lua
@@ -2,7 +2,7 @@
 
 local path = arg[0]:gsub('%.test%.lua', '')
 local suffix = package.cpath:match('?.(%a+);')
-package.cpath = ('%s/?.%s'):format(path, suffix)
+package.cpath = ('%s/?.%s;'):format(path, suffix)..package.cpath
 
 local jit_opt_default = {
     3, -- level
diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 4a7355a..8844b17 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -222,7 +222,6 @@ static int tracenum_base(lua_State *L)
         luaM_metrics(L, &metrics);
         assert(metrics.jit_trace_num == 0);
 
-        luaJIT_setmode(L, 0, LUAJIT_MODE_ON);
         lua_pushboolean(L, 1);
         return 1;
 }
===================================================================

-- 
Best regards,
Sergey Kaplun