Re: [Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Richard Henderson <rth@twiddle.net>]

On 06/29/2015 09:24 PM, Aurelien Jarno wrote:
> On 2015-06-29 10:28, Richard Henderson wrote:
>> On 06/25/2015 08:16 PM, Aurelien Jarno wrote:
>>>       for (shift = 4; (shift < 64) && bin; shift += 4) {
>>> -        int current_number = bin % 10;
>>> -
>>> -        dec |= (current_number) << shift;
>>> +        dec |= (bin % 10) << shift;
>>>           bin /= 10;
>>>       }
>>
>> You've changed from 32-bit division to 64-bit division just to solve a
>> problem with the shift.  Better to just change the type of current_number
>> there.
>
> Changing the type of current_number instead of bin would indeed solve
> the shift issue, but not the -2^31 case. As we take the absolute value,
> we need a 64-bit variable to hold the corresponding 2^31 value.
>

Ah, true enough.  I suppose adding a 32-bit unsigned variable with which to do 
the division is more trouble than it's worth.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~