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

[Markus Armbruster]

Blue Swirl <address@hidden> writes:

> On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <address@hidden> wrote:
>> On 26 June 2012 19:25, Blue Swirl <address@hidden> wrote:
>>> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <address@hidden> wrote:
>>>> On 26 June 2012 18:58, Blue Swirl <address@hidden> wrote:
>>>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <address@hidden> wrote:
>>>>>> +static inline uint64_t field64(uint64_t value, int start, int length)
>>>>>
>>>>> start and length could be unsigned.
>>>>
>>>> They could be, but is there any reason why they should be?
>>>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this
>>>> is consistent with that.
>>>
>>> Negative shifts don't work, the line with assert() would get shorter
>>> and simpler and I like unsigned values.
>>
>> I don't like using unsigned for numbers that merely happen to always
>> be positive (as opposed to actually requiring unsigned arithmetic)[*],
>> so I think I'll stick with being consistent with the existing bitops
>> functions, thanks :-)

Consistency is a strong argument.

> Using unsigned types also produces better code in some cases. There

Better code is an argument only if the effect can be demonstrated.

> are also operations which do not work well with signed integers (%,
> >>).

Operator >> is applicable here.  It works exactly as well for negative
right operand as it does for large positive right operand: undefined
behavior.

>> [*] the classic example of where that kind of thing can trip you up
>> is the way it complicates the termination condition on a "for (i = N;
>> i >= 0; i--)" decreasing loop.

Yup.  There are more, e.g. fun with unwanted truncation or sign
extension when mixing different integer types.  Stick to int unless you
have a compelling reason.