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

[Blue Swirl]

On Tue, Jul 10, 2012 at 7:37 PM, Peter Maydell <address@hidden> wrote:
> On 10 July 2012 20:18, Blue Swirl <address@hidden> wrote:
>> On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <address@hidden> wrote:
>>> There is no consensus.  I recognize the power of maintainers to force a
>>> change even without consensus.  Use it wisely.
>>
>> I thought I refuted all concrete arguments except performance.
>
> No, you made various claims that Markus and I at least
> disagreed with. (Conversely, we have made various claims
> that you disagree with -- this is what "no consensus" means...)

You did not present any concrete arguments. In this review you have
pointed at bugs in the assert() expression, thanks.

>
>> However, Avi kindly provided this part.
>
> One extra instruction, which isn't a load/store or branch? This is the
> worst kind of premature microoptimisation. If we're changing this
> it should be rooted in arguments about maintainability or similar,

I think signed and unsigned are pretty much equal in this respect.
Both signed and unsigned have corner cases. Unsigned has the advantage
of being more natural range for values which can't be negative by
their nature, like bit numbers. There's also the security argument
like Avi mentioned.

> or alternatively if it's really a performance improvement I'm
> sure you can produce the benchmarks that demonstrate that :-)
>
> [Plus the extract/deposit ops aren't doing array accesses!]

It's a clear benefit which signed integers can't provide, especially
on x86_64 which is an important platform. For me this clearly tips the
balance.

>
> -- PMM