Messages in this thread | | | Date | Mon, 01 Sep 2008 07:56:48 +0100 | From | "Jan Beulich" <> | Subject | Re: [PATCH] BUILD_BUG_ON() should not use array type |
| |
>> --- 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
| |