Re: [Qemu-devel] [PATCH v4 08/16] bitops: Add ONES macro

[Alistair Francis]

On Mon, Feb 29, 2016 at 3:51 AM, Alex Bennée <address@hidden> wrote:
>
> Alistair Francis <address@hidden> writes:
>
>> From: Peter Crosthwaite <address@hidden>
>>
>> Little macro that just gives you N ones (justified to LSB).
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>
>>  include/qemu/bitops.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index 8164225..27bf98d 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>> @@ -430,4 +430,6 @@ static inline uint64_t deposit64(uint64_t value, int 
>> start, int length,
>>      return (value & ~mask) | ((fieldval << start) & mask);
>>  }
>>
>> +#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1)
>> +
>
> I'm a little ambivalent about this one. The definition should probably
> be up at the top of the file with the other #defines. I don't like the
> name as it is a little too generic. I also notice in all the actual uses
> you immediately invert the result because you want to mask the top bits.
>
> I suspect what's needed here is a more generic helper:
>
> #define MAKE_64BIT_MASK (shift, length) \
> ...
>
> And then the:
>
>         .ro = ~ONES(27) },
>
> Becomes:
>
>         .ro = MAKE_64BIT_MASK(27, 64 - 27);
>
>
>>  #endif

I like that idea. I have updated the implementation to use this style
instead of the ONES() style.

Thanks,

Alistair

>
>
> --
> Alex Bennée
>