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