Re: [PATCH] BUILD_BUG_ON() should not use array type

[Jan Beulich]

>> --- linux-2.6.27-rc5/include/linux/kernel.h	2008-08-21 14:37:35.000000000 +0200
>> +++ 2.6.27-rc5-build-bug-on/include/linux/kernel.h	2008-08-28 15:10:28.000000000 +0200
>> @@ -468,13 +468,17 @@ struct sysinfo {
>>  };
>>  
>>  /* Force a compilation error if condition is true */
>> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
>> +#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
>> +
>> +/* Force a compilation error if condition is constant and true */
>> +#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
>
>MAYBE_BUILD_BUG_ON() hurts my brain.  It's doing:
>
>	if (__builtin_constant_p(expr))
>		BUILD_BUG_ON(expr);
>
>yes?  For inlined (or macro) callsites which can be used with constant
>or non-constant args.

Yes, its attempting to have the effect of that if() construct.

>It's tempting to just zap the one callsite and not add this at all, but
>I suppose that it's a legitimate thing to want to do, and that other
>users of it may well turn up.  Many of them are probably just using run-time
>BUG_ON(), which is sensible, but less efficient.
>
>However it would benefit from a clearer description and perhaps a
>better name.  BUILD_BUG_ON_IF_CONSTANT?

Admittedly I didn't like the name much either, but so I also didn't like
any other name I could think of. And really, I'd also favor removing
the one use site altogether (hence the comment about the construct
there not having any effect), but thought it'd be a step backward
documentation-wise.

So for re-submitting an adjusted patch I'd need to be clear about the
way to go...

>>  /* Force a compilation error if condition is true, but also produce a
>>     result (of value 0 and type size_t), so the expression can be used
>>     e.g. in a structure initializer (or where-ever else comma expressions
>>     aren't permitted). */
>> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>> +#define BUILD_BUG_ON_NULL(e) ((void *)BUILD_BUG_ON_ZERO(e))
>
>What does BUILD_BUG_ON_NULL() do and why did you add it?  It has no
>callers.

I was under the (apparently wrong) impression that while plain 0 can be
used to initialize a pointer, an expression evaluating to zero would need
a cast. While that's not the case as I just checked, I'd still prefer keeping
the construct both for documentation purposes and because in the error
case without the case you'd not only get the intended error message,
but also an extra (and perhaps confusing) warning.

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/