Re: [Qemu-devel] [PATCH v2] utils: Add pow2ceil()

[Eric Blake]

On 02/23/2015 10:40 AM, Markus Armbruster wrote:

>>> int64_t pow2floor(int64_t value)
>>> {
>>>     assert(value > 0);
>>>     return 0x8000000000000000u >> clz64(value);
>>> }
>>
>> Needs to be 0x8000000000000000ull for 32-bit machines to compile correctly.
> 
> Why?

Because 0x8000000000000000u is only required to be a 'long', and on
32-bit machines, your constant would overflow long.  See, for example,
commit 5cb6be2ca.  You NEED the 'll' suffix to ensure that the compiler
doesn't reject the constant as an overflow.

> 
>> Why is the parameter int64_t?  Wouldn't it be more useful to have:
>>
>> uint64_t pow2floor(uint64_t value)
> 
> Crossed my mind, too.  However, the existing callers pass *signed*
> arguments.

I guess it means auditing callers either way.

> 
>>> int64_t pow2ceil(int64_t value)
>>> {
>>
>> Again, why allow signed inputs?
>>
>>>     assert(value <= 0x4000000000000000)
>>>     if (value <= 1)
>>>     return 1;
>>
>> In particular, this slams all negative values to a result of 1, which
>> doesn't necessarily make sense.
> 
> It implements a straightforward contract: return the smallest power of
> two greater or equal to the argument.  The function's domain is the set
> of int64_t arguments where this value can be represented in int64_t,
> i.e. [-2^63..2^62].
> 
> Feel free to suggest a more sensible contract.

But why should I claim that the nearest power of 2 greater than -3 is
positive 1, when I could argue that it should instead be -2 (nearest
positive or negative power of 2 rounding towards +inf) or -4 (nearest
positive or negative power of 2 rounding away from 0)?  Since there are
multiple potential contracts once negative values are involved, I find
it easier to just make the contract require a positive input to begin with.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature