Xen project Mailing List

Eric Blake <eblake@xxxxxxxxxx> writes: > On Fri, Sep 01, 2023 at 10:48:26AM +0200, Markus Armbruster wrote: >> > Indeed, not fully understanding the preprocessor makes for some >> > interesting death traps. >> >> We use ALL_CAPS for macros to signal "watch out for traps". >> > >> >> -#define QOBJECT(obj) ({ \ >> >> - typeof(obj) _obj = (obj); \ >> >> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ >> >> +#define QOBJECT_INTERNAL(obj, l) ({ \ >> >> + typeof(obj) PASTE(_obj, l) = (obj); \ >> >> + PASTE(_obj, l) \ >> >> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \ >> >> }) >> >> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__) >> > >> > Slick! Every call to QOBJECT() defines a unique temporary variable >> > name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is >> > 1): >> > >> > ({ >> > typeof((o)) _obj1 = ((o)); >> > _obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL; >> > }) >> > >> > which has three sets of redundant parens that could be elided. Why do >> > you need to add () around obj when forwarding to QOBJECT_INTERNAL()? >> >> Habit: enclose macro arguments in parenthesis unless there's a specific >> need not to. >> >> > The only way the expansion of the text passed through the 'obj' >> > parameter can contain a comma is if the user has already parenthesized >> > it on their end (not that I expect a comma expression to be a frequent >> > argument to QOBJECT(), but who knows). Arguing that it is to silence >> > a syntax checker is weak; since we must NOT add parens around the >> > parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is >> > obviously wrong). >> > >> > Meanwhile, the repetition of three calls to PASTE() is annoying. How >> > about: >> > >> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \ >> > typeof _obj _tmp = _obj; \ >> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \ >> > )} >> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp) >> > #define QOBJECT_INTERNAL(_arg, _ctr) \ >> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr)) >> > #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__) >> > >> > or: >> > >> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \ >> > typeof(_obj) _tmp = (_obj); \ >> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \ >> > )} >> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp) >> > #define QOBJECT_INTERNAL(_arg, _ctr) \ >> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr)) >> > #define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__) >> > >> > where, in either case, QOBJECT(o) should then expand to >> > >> > ({ >> > typeof (o) _obj1 = (o); >> > _obj1 ? container_of(&_obj1->base, QObject, base) : NULL; >> > }) >> >> The idea is to have the innermost macro take the variable name instead >> of the counter. "PASTE(_obj, l)" then becomes "_tmp" there, which is >> more legible. However, we pay for it by going through two more macros. >> >> Can you explain why you need two more? > > Likewise habit, on my part (and laziness - I wrote the previous email > without testing anything). I've learned over the years that pasting is > hard (you can't mix ## with a macro that you want expanded without 2 > layers of indirection), so I started out by adding two layers of > QOBJECT_INTERNAL, then wrote QOBJECT to forward to QOBJECT_INTERNAL. > But now that you asked, I actually spent the time to test with the > preprocessor, and your hunch is indeed correct: I over-compensated > becaues of my habit. > > $cat foo.c > #define PASTE(_a, _b) _a##_b > > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \ > typeof _obj _tmp = _obj; \ > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \ > )} > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp) > #define QOBJECT_INTERNAL(_arg, _ctr) \ > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr)) > #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__) > > QOBJECT(o) > > #define Q_INTERNAL_1(_obj, _tmp) ({ \ > typeof _obj _tmp = _obj; \ > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \ > )} > #define Q_INTERNAL(_arg, _ctr) \ > Q_INTERNAL_1(_arg, PASTE(_obj, _ctr)) > #define Q(obj) Q_INTERNAL((obj), __COUNTER__) > > Q(o) > $ gcc -E foo.c > # 0 "foo.c" > # 0 "<built-in>" > # 0 "<command-line>" > # 1 "/usr/include/stdc-predef.h" 1 3 4 > # 0 "<command-line>" 2 > # 1 "foo.c" > # 12 "foo.c" > ({ typeof (o) _obj0 = (o); _obj0 ? container_of(&_obj0->base, QObject, base) > : NULL; )} > # 22 "foo.c" > ({ typeof (o) _obj1 = (o); _obj1 ? container_of(&_obj1->base, QObject, base) > : NULL; )} > > So the important part was merely ensuring that __COUNTER__ has a > chance to be expanded PRIOR to the point where ## gets its argument, > and one less layer of indirection was still sufficient for that. The version with less indirection is easier to understand for me. >> >> Complication: the innermost macro needs to take all its variable names >> as arguments. The macro above has just one. Macros below have two. >> >> Not quite sure which version is easier to understand. > > Proper comments may help; once you realize that you are passing in the > unique variable name(s) to be used as the temporary identifier(s), > rather than an integer that still needs to be glued, then separating > the task of generating name(s) (which is done once per name, instead > of repeated 3 times) makes sense to me. I also like minimizing the > number of times I have to spell out __COUNTER__; wrapping unique name > generation behind a well-named helper macro gives a better view of why > it is needed in the first place. I can add this comment to every instance of the __COUNTER__ trick: /* * Preprocessor wizardry ahead: glue(_val, l) expands to a new * identifier in each macro expansion. Helps avoid shadowing * variables and the unwanted name captures that come with it. */ > At any rate, this comment still applies: > >> > >> > I think you are definitely on the right track to have all internal >> > variable declarations within a macro definition use multi-layered >> > expansion with the help of __COUNTER__ to ensure that the macro's >> > temporary variable is globally unique; so if you leave it as written, >> > I could live with: >> > >> > Reviewed-by: Eric Blake <eblake@xxxxxxxxxx> >> > >> > But if you respin it to pick up any of my suggestions, I'll definitely >> > spend another review cycle on the result. >> > >> > If it helps, here's what I ended up using in nbdkit: >> > >> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36 >> > /* https://stackoverflow.com/a/1597129 >> > * https://stackoverflow.com/a/12711226 >> > */ >> > #define XXUNIQUE_NAME(name, counter) name ## counter >> > #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter) >> > #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__) >> > >> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47 >> > #include "unique-name.h" >> > >> > #undef MIN >> > #define MIN(x, y) \ >> > MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y)) >> > >> > ... >> > #define MIN_1(x, y, _x, _y) ({ \ >> > __auto_type _x = (x); \ >> > __auto_type _y = (y); \ >> > _x < _y ? _x : _y; \ >> > }) >> >> Thanks! > > and so I'm fine leaving it up to you if you want to copy the paradigm > I've seen elsewhere, or if you want to leave it as you first proposed. > The end result is the same, and it is more of a judgment call on which > form is easier for you to reason about. I need to review the two competing versions once more to decide which I find easier to read. Or shall I say less hard; preprocessor wizardry is never really easy. > But as other commenters mentioned, we already have a glue() macro, so > use that instead of PASTE(), no matter what else you choose. > > Looking forward to v2! Thanks again!

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Re: [PATCH 7/7] qobject atomics osdep: Make a few macros more hygienic