qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types
Date: Thu, 12 Jul 2012 22:05:21 +0100

On 12 July 2012 21:18, Blue Swirl <address@hidden> wrote:
> On Tue, Jul 10, 2012 at 9:36 PM, Peter Maydell <address@hidden> wrote:
>> Basically 'int' has more natural
>> behaviour for reasoning about than 'unsigned' in ranges
>> where it's usually used (ie small ones).
>
> But 'unsigned' is much more naturally suited to values that can't be
> negative. This part is bikeshedding, your point of view against mine.

I agree that this is all veering quite close to bikeshedding, so
this email is my last on the subject.

So, both 'unsigned' and 'int' have two discontinuities, at the extreme
ends of their ranges. For 'unsigned' these are at 0 and UINT_MAX;
for 'int' they're at INT_MIN and INT_MAX. Discontinuities are bad
because they break our intuitive reasoning about the behaviour of
integers and result in bugs which can easily slip through code review.
Any time you might be near a discontinuity you have to think carefully
about what is actually going to happen. Sometimes that's unavoidable
whatever type you pick (eg in situations where values are coming from
an untrusted source which might be trying to find a security hole).
But often values are from another trusted area of the code (or even
obviously small by inspection of the immediate area). In this case
'int' is good because it keeps the discontinuities well away from the
range you'll be working in. Because 'unsigned' has a discontinuity
near 0 it's much easier to bump into it even for numbers in the
expected use range. So when you're really just dealing with "small
numbers that happen to be positive" and don't need the unsigned
arithmetic semantics, 'int' is the better choice.
I think the maintainability/reduced-bugginess argument hugely
outweighs minor details of one insn more or less in some operations.

(If we care about that, we should be much more in favour of using 'int'
returns rather than 'bool', since the bool return forces the compiler
to insert code which normalises a zero-or-nonzero value to zero-or-one.
But there too the readability and maintainability benefits are worth
taking the unmeasurably tiny extra execution time.)

> Currently some functions use 'unsigned long' for bit numbers,
> shouldn't they be changed to 'int' in your logic?

I think that falls into the category of things I would recommend
changing in code review, or if fixing some related kind of
overflow bug, but would not change existing working code for.

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]