Re: [Qemu-devel] [PATCH v2] bitops.h: Add field32() and field64() functions to extract bitfields

[Avi Kivity]

On 06/27/2012 04:28 PM, Eric Blake wrote:
> On 06/27/2012 07:15 AM, Avi Kivity wrote:
>> On 06/27/2012 01:29 PM, Peter Maydell wrote:
>>> Add field32() and field64() functions which extract a particular
>>> bit field from a word and return it. Based on an idea by Jia Liu.
>>>
> 
>>> +static inline uint64_t field64(uint64_t value, int start, int length)
>>> +{
>>> +    assert(start >= 0 && start <= 63 && length > 0 && start + length <= 
>>> 64);
>>> +    return (value >> start) & (~0ULL >> (64 - length));
>>> +}
>>> +
>> 
>> Undefined for length == 64, so better add that to the assert.
> 
> How so?  ~0ULL >> 0 is well-defined (a length of 64, coupled with a
> start of 0, is effectively writing the entire uint64_t value).  While it
> is unlikely that anyone would ever trigger this with start and length
> being constants (field64(value, 0, 64) == value), it might be triggered
> when start and length are computed from other code.

I was confused with length == 0.

> 
>> 
>> I suggest adding the analogous functions for writing.  I believe the
>> common naming is extract/deposit.
>> 
>> static inline uint64_t deposit64(uint64_t *pvalue, unsigned start,
>>                                  unsigned length, uint64_t fieldval)
>> {
>>     uint64_t mask = (((uint64_t)1 << length) - 1) << start;
> 
> Here, a length of 64 is indeed undefined, but for symmetry with allowing
> a full 64-bit extraction from a start of bit 0, you could special case
> the computation of that mask.
> 
>>     *pvalue = (*pvalue & ~mask) | ((fieldval << start) & mask);
> 
> As with the extraction function, it would be worth an assert that start
> and length don't trigger undefined shifts.  It may be further worth
> asserting that fieldval is within the range given by length, although I
> could see cases of intentionally wanting to pass in -1 to set all bits
> within the mask and a tight assert would prevent that usage.
> 
> You marked this function signature as returning uint64_t, but didn't
> return anything.  I think the logical return is the new contents of
> *pvalue.  Another logical choice would be marking the function void.
> 

Void makes more sense here as you usually want in-place update.


-- 
error compiling committee.c: too many arguments to function