From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 331DD469719 for ; Wed, 7 Oct 2020 21:34:12 +0300 (MSK) Date: Wed, 7 Oct 2020 21:23:35 +0300 From: Igor Munkin Message-ID: <20201007182335.GQ18920@tarantool.org> References: <87bf84f266e16cd9401394c8276b1cd97f91a82d.1601878708.git.skaplun@tarantool.org> <20201006221721.GN18920@tarantool.org> <20201007143500.GA27749@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201007143500.GA27749@root> Subject: Re: [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for platform metrics List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.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. > > > > > 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. > > > 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 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 . > > 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 > @@ -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); > =================================================================== > > -- > Best regards, > Sergey Kaplun -- Best regards, IM