Re: [Libguestfs] [nbdkit] ARRAY_SIZE [was: vddk: Demote another "phone home" error message to debug]

On Thu, Jul 07, 2022 at 12:58:53PM +0200, Laszlo Ersek wrote: I see that is now done as part of 0fa23df5c. While include/array-size.h looks okay, I personally have a tough time with include/compiler-macros.h BUILD_BUG_ON_ZERO(expr). Every time I encounter a similarly-named macro in other projects, I have to go re-read the docs, because (to at least me) the name is not self-describing. Quoting the patch itself: +/* This expression fails at compile time if 'expr' is true. It does + * this by constructing a struct which has an impossible + * (negative-sized) array. + * + * If 'expr' is false then we subtract the sizes of the two identical + * structures, returning zero. + */ My summary of those docs: BUILD_BUG_ON_ZERO(1) is a build bug (forced compiler error); BUILD_BUG_ON_ZERO(0) is 0. To use the macro, I then have to come up with a condition that results in false to avoid a build bug (when normally, I like to come up with predicates that result in true when I want to proceed, rather than predicates that result in false). To me, the name "BUILD_BUG_ON_ZERO" parses as "I want a build bug if I pass in 0", and not "I want to use 0 unchanged, and anything else to be a build bug". And once I get over my mis-parse of the name, I then have to remember that ARRAY_SIZE must use '+ BUILD_BUG_ON_ZERO (!TYPE_IS_ARRAY(a))' to mean "add 0 if 'a' is an array type, because '!ARRAY_TYPE(a)' is 0", which involved a double negative. I don't mind if we keep the macro, but can we please use a better name than the kernel folks have stuck themselves with, especially since we are NOT copying the kernel's implementation of the macro (as our licensing won't allow it in the first place)? I'm also worried that your code uses 'struct { int _array_size_failed[(expr)...]; }', which may end up causing VLA situations, where (mistakenly) passing in a non-constant expr could compile (as a VLA) rather than letting the compiler diagnose that expr wasn't consant. Hence, I like bitfields better than array sizes when trying to force a conditional compiler error, as it gets VLA out of the picture. If I were designing it from scratch, I might create: /* If EXPR is true, produce a non-zero struct size. Otherwise, fail the build */ #define BUILD_BUG_STRUCT_SIZE(expr) \ (sizeof (struct { int: (expr) ? 1 : -1; })) /* If EXPR is true, produce 0. Otherwise, fail the build */ #define BUILD_BUG_UNLESS_TRUE(expr) \ (!BUILD_BUG_STRUCT_SIZE(expr)) at which point the definition of ARRAY_SIZE(a) includes '+ BUILD_BUG_UNLESS_TRUE(TYPE_IS_ARRAY(a))', which is easier (for me) to read. After writing the above, I also looked at how gnulib did things in lib/verify.h. Their arugment is that no single macro can be used in all contexts, so they provide: /* R must be non-zero, or compilation fails. */ verify_expr(R, E) /* for use in scalar contexts, result in E */ verify(R) /* for use in declaration contexts */ but it matches my bias of avoiding the double-negative (I find it easy to write a predicate that must either be true or will cause compilation failure). Here, I would define ARRAY_SIZE by using '+ verify_expr(TYPE_IS_ARRAY(a), 0)' [Caution - I have touched gnulib's lib/verify.h in the past. Although much of it has been rewritten in the meantime as compilers have progressed, I may still be tainted enough by my past work on gnulib's implementation of compile-time checking that I don't trust myself to do a genuinely clean-room reimplementation compatible with nbdkit's licensing needs] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org