From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 9F5724765E0 for ; Mon, 21 Dec 2020 01:46:07 +0300 (MSK) Date: Mon, 21 Dec 2020 01:46:02 +0300 From: Igor Munkin Message-ID: <20201220224602.GR5396@tarantool.org> References: <99c1138ecb4d0d76873409793ad1fbfaa0b449e3.1608142899.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <99c1138ecb4d0d76873409793ad1fbfaa0b449e3.1608142899.git.skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v1 07/11] debug: move debug_frameline to public module API 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 patch! I doubt this change need to be made as a separate patch. Anyway, I left a couple of nits below. Furthermore, it's better to use 'core' prefix here, otherwise it's not needed since every translation unit uses own one then. On 16.12.20, Sergey Kaplun wrote: > This patch renames debug_frameline to lj_debug_frameline and moves it to > public module API (does not provide it with LUA_API). It > will be used for memory profiler in the next patches. Minor: I propose more simple wording: | core: make debug_frameline visible for other sources | | This change makes debug_frameline function LuaJIT-wide visible to be | used in other subsystems (e.g. memory profiler). > > Part of tarantool/tarantool#5442 > --- > src/lj_debug.c | 8 ++++---- > src/lj_debug.h | 1 + > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/src/lj_debug.h b/src/lj_debug.h > index 5917c00..1b5ef29 100644 > --- a/src/lj_debug.h > +++ b/src/lj_debug.h > @@ -40,6 +40,7 @@ LJ_FUNC void lj_debug_addloc(lua_State *L, const char *msg, > LJ_FUNC void lj_debug_pushloc(lua_State *L, GCproto *pt, BCPos pc); > LJ_FUNC int lj_debug_getinfo(lua_State *L, const char *what, lj_Debug *ar, > int ext); > +LJ_FUNC BCLine lj_debug_frameline(lua_State *L, GCfunc *fn, cTValue *nextframe); Minor: As you can see below, is provided only if profiler is enabled. Since this function is necessary only for memprof, there is no need to make it LuaJIT-wide visible by default. This looks more natural to me. This is not required but seems like a nice approach, so feel free to ignore. > #if LJ_HASPROFILE > LJ_FUNC void lj_debug_dumpstack(lua_State *L, SBuf *sb, const char *fmt, > int depth); > -- > 2.28.0 > -- Best regards, IM