From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 9C2D8469719 for ; Wed, 30 Sep 2020 09:38:00 +0300 (MSK) Date: Wed, 30 Sep 2020 09:27:25 +0300 From: Igor Munkin Message-ID: <20200930062725.GA18920@tarantool.org> References: <6b7def49c9d2252425d3944cc4274c9a155ae9e9.1600862684.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 2/2] lua: abort trace recording on fiber yield List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for your review! On 30.09.20, Vladislav Shpilevoy wrote: > Thanks for the patch! > > Functionally all looks good. See one comment related not to Lua. > > > diff --git a/src/lua/utils.c b/src/lua/utils.c > > index 8e98f7607..6e38f5734 100644 > > --- a/src/lua/utils.c > > +++ b/src/lua/utils.c > > @@ -1309,10 +1310,61 @@ tarantool_lua_utils_init(struct lua_State *L) > > return 0; > > } > > > > +/* > > + * XXX: There is already defined macro in say.h header > > + * (included in diag.h). As a result the call below is misexpanded > > + * and compilation fails with the corresponding error. To avoid > > + * this error the macro is temporary renamed and restored later. > > + * Compilation now fails for the following cases: > > + * * temporary name is used in scope of this > > + * translation unit > > + * * is not define, so this hack can be freely dropped > > + */ > > +#if defined(panic) && !defined(gh1700_panic) > > +# define gh1700_panic panic > > +# undef panic > > +#else > > +# error "Can't redefine macro" > > +#endif > > Unfortunately, it won't work. Macros are not variables. You > can's assign them like that. I wish we could. > > What you did here is you eliminated 'panic' macro, and added a > new macro 'gh1700_panic' whose value is "panic". Not what was > in the old 'panic', but exactly "panic". So the old value of 'panic' > is lost here. Hell, you're right. They can be recursively expanded, but if panic macro is undefined then it is just a bareword (as I originally wanted). > > > Also when you try to restore the old 'panic', it also won't work > again. As an alternative you can just do '#undef panic', since your > code is in the end of the file anyway, it won't break shit. Or make > a separate patch, which would turn panic() into a function. I simply undefined macro (the diff is below) and filed an issue[1] to rewrite this part. ================================================================================ diff --git a/src/lua/utils.c b/src/lua/utils.c index 6e38f5734..bb2287162 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -1314,18 +1314,10 @@ tarantool_lua_utils_init(struct lua_State *L) * XXX: There is already defined macro in say.h header * (included in diag.h). As a result the call below is misexpanded * and compilation fails with the corresponding error. To avoid - * this error the macro is temporary renamed and restored later. - * Compilation now fails for the following cases: - * * temporary name is used in scope of this - * translation unit - * * is not define, so this hack can be freely dropped + * this error the macro is undefined since it's not used anymore + * in scope of this translation unit. */ -#if defined(panic) && !defined(gh1700_panic) -# define gh1700_panic panic -# undef panic -#else -# error "Can't redefine macro" -#endif +#undef panic /** * This routine encloses the checks and actions to be done when @@ -1364,7 +1356,3 @@ void cord_on_yield(void) */ lj_trace_abort(g); } - -/* Restore macro back */ -#define panic gh1700_panic -#undef gh1700_panic ================================================================================ [1]: https://github.com/tarantool/tarantool/issues/5365 -- Best regards, IM