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

[Blue Swirl]

On Thu, Jun 28, 2012 at 5:58 AM, Markus Armbruster <address@hidden> wrote:
> Blue Swirl <address@hidden> writes:
>
>> 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.
>
> Hearsay.  Your honor, I rest my case :)

Objection: the internet can be a veritable source.

>>>> 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.
>
> No, my point is: with unsigned the negative case turns into the large
> positive case, which is exactly as bad.

Only if you use signed integers in the first place and then are also
careless about conversion.

>>>>> [*] 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.
>
> Leads to much dangerous mixing of different integer types, and ugly
> casts all over the place.

The same argument would also apply to integers of different sizes, use
of which can cause dangerous mixing and ugly casts too. But it's not
possible to use only signed or unsigned types (and never the other)
for a program with size of QEMU. In this case if only one type would
be allowed, it would have to be uintptr_t since we absolutely need
that but then the set of arithmetic operations allowed could be too
limited.

>> 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.
>
> Narrow parameter types only hide programming errors here, because
> arguments get silently truncated before they can reach the protective
> assertion.

We could enable the compiler warning about truncating and sign
changing operations, -Wconversion to find the suspect cases.