Am Freitag, dem 06.12.2024 um 02:25 +0000 schrieb David Laight: > From: Vincent Mailhol > > Sent: 05 December 2024 15:31 > > > > -CC: Martin Uecker <Martin.Uecker@xxxxxxxxxxxxxxxxxxxxx> > > +CC: Martin Uecker <muecker@xxxxxxx> > > (seems that Martin changed his address) My current one is this: uecker@xxxxxxxxx Martin > > > > On Thu. 5 Dec. 2024 at 03:39, David Laight <David.Laight@xxxxxxxxxx> wrote: > > > > Sent: 02 December 2024 17:33 > > > > > > > > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > > > > > __is_constexpr(), while being one of the most glorious one liner hack > > > > ever witnessed by mankind, is overly complex. Following the adoption > > > > of C11 in the kernel, this macro can be simplified through the use of > > > > a _Generic() selection. > > > > > > You should give credit to some of the earlier patches that do the same. > > > I'm sure there were some related ones from Linus - not applied yet. > > > > ACK. Would adding a suggested--by Linus tag solve your concern? > > I actually suspect the first patches to change __is_constexpr() to > use _Generic were from myself. > I've found a patch I send in November 2023. > > > > > > > First, split the macro in two: > > > > > > > > - __is_const_zero(x): an helper macro; tells whether x is the > > > > integer constant expression 0 or something else. > > > > > > > > - is_const(x): replacement of __is_constexpr(); tells whether x is a > > > > integer constant expression. > > > > > > > > The split serves two purposes: first make it easier to understand; > > > > second, __is_const_zero() will be reused as a building block for other > > > > is_const_*() macros that will be introduced later on. > > > > > > > > The core principle of __is_constexpr() to abuse the return type of the > > > > ternary operator remains, but all the surrounding sizeof() hack > > > > disappear. > > > > > > > > On a side note, while not relevant to the kernel, __is_constexpr() > > > > relied on the GNU extension that sizeof(void) is 1. const_expr() does > > > > not use any GNU extensions, making it ISO C compliant. > > > > > > > > __is_constexpr() is temporarily kept and will be removed once all its > > > > users get migrated to is_const() (or one of its friend). > > > > > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > --- > > > > include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 41 insertions(+) > > > > > > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > > > index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644 > > > > --- a/include/linux/compiler.h > > > > +++ b/include/linux/compiler.h > > > > @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off) > > > > #define statically_true(x) (__builtin_constant_p(x) && (x)) > > > > #define statically_false(x) (__builtin_constant_p(x) && (x) == 0) > > > > > > > > +/* > > > > + * Whether x is the integer constant expression 0 or something else. > > > > + * > > > > + * Details: > > > > + * - The C11 standard defines in §6.3.2.3.3 > > > > + * (void *)<integer constant expression with the value 0> > > > > + * as a null pointer constant (c.f. the NULL macro). > > > > + * - If x evaluates to the integer constant expression 0, > > > > + * (void *)(x) > > > > + * is a null pointer constant. Else, it is a void * expression. > > > > + * - In a ternary expression: > > > > + * condition ? operand1 : operand2 > > > > + * if one of the two operands is of type void * and the other one > > > > + * some other pointer type, the C11 standard defines in §6.5.15.6 > > > > + * the resulting type as below: > > > > + * if one operand is a null pointer constant, the result has the > > > > + * type of the other operand; otherwise [...] the result type is > > > > + * a pointer to an appropriately qualified version of void. > > > > + * - As such, in > > > > + * 0 ? (void *)(x) : (char *)0 > > > > + * if x is the integer constant expression 0, operand1 is a null > > > > + * pointer constant and the resulting type is that of operand2: > > > > + * char *. If x is anything else, the type is void *. > > > > + * - The (long) cast silences a compiler warning for when x is not 0. > > > > + * - Finally, the _Generic() dispatches the resulting type into a > > > > + * Boolean. > > > > > > The comment is absolutely excessive. > > > I'm sure I managed about 2 lines in one of the patches I did. > > > > I think that Linus made it clear in: > > > > https://lore.kernel.org/all/CAHk-=wgfpLdt7SFFGcByTfHdkvv7AEa3MDu_s_W1kfOxQs49pw@xxxxxxxxxxxxxx/ > > > > that this deserves a detailed comment. > > And he wrote one in https://lore.kernel.org/all/CAHk-=wiq=GUNWJwWh1CRAYchW73UmOaSkaCovLatfDKeveZctA@xxxxxxxxxxxxxx/ > > /* > * iff 'x' is a non-zero constant integer expression, > * then '!(x)' will be a zero constant integer expression, > * and casting that to 'void *' will result in a NULL pointer. > * Otherwise casting it to 'void *' will be just a regular 'void *'. > * > * The type of '0 ? NULL : (char *)' is 'char *' > * The type of '0 ? (void *) : (char *) is 'void *' > */ > #define const_true(x) \ > _Generic(0 ? (void *)((long)!(x)) : (char *)0, char *: 1, void *: 0) > > > > > > > The details block in the current __is_constexpr() is 37 lines long, > > the details block in __is_const_zero() takes 22 lines. So I would > > argue that I made things better. > > The old block was too long :-) > > > > > Unless more people share your concern, I am planning to keep this comment as-is. > > > > > > + * > > > > + * Glory to Martin Uecker <Martin.Uecker@xxxxxxxxxxxxxxxxxxxxx> > > > > > > IIRC Martin has agreed in the past that the accreditation can > > > be removed - especially since it refers to the 'sizeof (void)' trick. > > > > I tried to look for such message: > > > > https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr > > > > but couldn't find it. Do you have the link? > > > > @Martin, do you agree that I remove the accreditation? > > > > > > + */ > > > > +#define __is_const_zero(x) \ > > > > + _Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0) > > > > + > > > > +/* > > > > + * Returns a constant expression while determining if its argument is a > > > > + * constant expression, most importantly without evaluating the argument. > > > > > > You need to differentiate between a 'constant integer expression' > > > and a 'compile time constant'. > > > > OK. This one was just copied from the previous __is_constexpr(). I will apply > > "s/constant expression/constant integer expression/g" in v2. > > > > > > + * > > > > + * If getting a constant expression is not relevant to you, use the more > > > > + * powerful __builtin_constant_p() instead. > > > > > > __builtin_constant_p() is not 'more powerful' it is testing for > > > something different. > > > > I meant to say that __builtin_constant_p() is more powerful at > > constant folding. But I agree that the comment is not clear. > > > > What about this? > > > > If getting a constant integer expression is not relevant to you, use > > __builtin_constant_p() which not only returns true if the argument > > is an integer constant expression, but also if it is a compile time > > constant. > > Complete f***ed tense. > > It's not about 'constant folding' and 'powerful' isn't the correct word. > They are checking for two different things. > > A 'constant integer expression' is defined by the C language, and is > basically something that is constant when first parsed by the compiler > (my definition) so it can pretty much only contain constants, sizeof() > and offsetof(). > > __builtin_constant_p() is true if the compiler decides that an expression is > constant. This can track values through inlined function calls and can > change from 'unknown' to 'true' late in the compilation. > > David > > > > > > > Yours sincerely, > > Vincent Mailhol > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
