Re: [Xen-devel] [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.02.14 at 15:27, Tim Deegan <tim@xxxxxxx> wrote:
> No semantic changes, just makes the control flow a bit clearer.
> 
> I was looking at this bcause the (-!__builtin_constant_p(x) | x__)
> formula is too clever for Coverity, but in fact it always takes me a
> minute or two to understand it too. :)
> 
> Signed-off-by: Tim Deegan <tim@xxxxxxx>

Well, I'm not really happy to see this changed into more text
(even if fewer lines), because to me that's part of what makes
such macros badly readable, but ...

>  xen/include/asm-x86/bitops.h | 62 
> ++++++++++++++++++++------------------------
>  xen/include/xen/bitmap.h     | 30 ++++++++++++---------
>  2 files changed, 46 insertions(+), 46 deletions(-)

... at least the overall change is not growing the number of
lines.

Nevertheless a small consistency nit:

> +#define find_next_bit(addr, size, off) ({                                   \
> +    unsigned int r__;                                                       \
> +    const unsigned long *a__ = (addr);                                      \
> +    unsigned int s__ = (size);                                              \
> +    unsigned int o__ = (off);                                               \
> +    if ( __builtin_constant_p(size) && s__ == 0 )                           \

Using == 0 here, ...

> +        r__ = s__;                                                          \
> +    else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG )          \
> +        r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__);   \
> +    else if ( __builtin_constant_p(off) && !o__ )                           \

... but using ! here. I'd prefer the latter everywhere, but I'd also
be fine with you choosing the former consistently. (Same of course
again further down.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel