[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] bitops.h: Add field32() and field64() functi
Re: [Qemu-devel] [PATCH v2] bitops.h: Add field32() and field64() functions to extract bitfields
Wed, 27 Jun 2012 17:01:50 +0300
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0
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 <=
>>> + 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