From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 7E6094765E0 for ; Thu, 24 Dec 2020 09:51:15 +0300 (MSK) Date: Thu, 24 Dec 2020 09:50:30 +0300 From: Sergey Kaplun Message-ID: <20201224065030.GJ9101@root> References: <99c1138ecb4d0d76873409793ad1fbfaa0b449e3.1608142899.git.skaplun@tarantool.org> <20201220224602.GR5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201220224602.GR5396@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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, Thanks for the review! On 21.12.20, Igor Munkin wrote: > 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). I'll squash it with memprof commit taking into account your comment below. Add this note as a separate paragraph. > > > > > 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. Looks really nice, I'll fix this in the next series. > > > #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 -- Best regards, Sergey Kaplun