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

[Blue Swirl]

On Wed, Jun 27, 2012 at 6:01 AM, Markus Armbruster <address@hidden> wrote:
> 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.

Yes, but if using unsigned ints for the all bit ops makes sense, they
all should be converted. This case could start using unsigned ints
before that. It's the same as CODING_STYLE.

>
>> Using unsigned types also produces better code in some cases. There
>
> Better code is an argument only if the effect can be demonstrated.

I don't know even for which compilers or CPUs this is true so it's
unlikely I could demonstrate it. However, googling finds a few
articles in defense of this.

>
>> 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.

Score one for the unsigned which avoids the negative case.

>>> [*] 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.

Yes, mixing signedness is not fun.

>  Stick to int unless you
> have a compelling reason.

I've seen exactly opposite guidance: use unsigned whenever possible.

In this case, using signed values for bit numbers is never useful. The
bit numbers simply are not meaningful if negative, so unsigned values
are more intuitive. The types could be even smaller, like uint8_t.