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

[Igor Munkin <imun@tarantool.org>]

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/clib-misclib-getmetrics.test.lua b/test/clib-misclib-getmetrics.test.lua
> > 
> > Add Github issue to the file name.
> 
> Here and below:
> I thought we use gh-xxxx style for bugfix tests. Tests for new
> features are named the same as a feature.
> Is `misclib-getmetrics-{capi,lapi}.test.lua` good enough?
> 

Yes, I'm OK with these names.

> > 
> > > new file mode 100755
> > > index 0000000..34adaba
> > > --- /dev/null
> > > +++ b/test/clib-misclib-getmetrics.test.lua
> > > @@ -0,0 +1,174 @@
> > > +#!/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
> > 
> > I've already solved this issue, so this part can be easily borrowed from
> > utils.lua[1]. Consider the following:
> > | local path = arg[0]:gsub('%.test%.lua', '')
> > | local suffix = package.cpath:match('?.(%a+);')
> > | package.cpath = ('%s/?.%s;%s'):format(path, suffix, package.cpath)
> > 
> 
> Yep. See now. Thanks! We should add one small function for this
> to reuse in other tests later.

Yes, this part can be moved to a separate routine in utils.lua.

> 

<snipped>

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

<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>.

>  

<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.

>  	/* Generate traces. */
>  	lua_call(L, 0, 1);
>  	n = lua_gettop(L);

<snipped>

> ===================================================================
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM