Re: [Tarantool-patches] [PATCH v2 2/2] lua: abort trace recording on fiber yield

[Igor Munkin <imun@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 <panic> 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 <gh1700_panic> is used in scope of this
> > + *   translation unit
> > + * * <panic> 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 <panic> 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).

> 

<snipped>

> 
> 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 <panic> 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 <panic> 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 <gh1700_panic> is used in scope of this
- *   translation unit
- * * <panic> 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 <panic> 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 <panic> macro back */
-#define panic gh1700_panic
-#undef gh1700_panic

================================================================================

[1]: https://github.com/tarantool/tarantool/issues/5365

-- 
Best regards,
IM