Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 23/10/2019 à 16:23, Eric Blake a écrit :
> On 10/23/19 9:15 AM, Frediano Ziglio wrote:
>>>
>>> On 10/23/19 8:42 AM, Laurent Vivier wrote:
>>>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>>>>> Signed-off-by: Frediano Ziglio <address@hidden>
>>>>> ---
>>>>>    util/qemu-timer.c | 6 +-----
>>>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
>>>>> index d428fec567..094a20a05a 100644
>>>>> --- a/util/qemu-timer.c
>>>>> +++ b/util/qemu-timer.c
>>>>> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>>>>>        ms = DIV_ROUND_UP(ns, SCALE_MS);
>>>>>           /* To avoid overflow problems, limit this to 2^31, i.e.
>>>>> approx 25
>>>>>        days */
>>>>> -    if (ms > (int64_t) INT32_MAX) {
>>>>> -        ms = INT32_MAX;
>>>>> -    }
>>>>> -
>>>>> -    return (int) ms;
>>>>> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>>>>>    }
>>>
>>> Why so many casts?  It should also work as:
>>>
>>> return MIN(ms, INT32_MAX);
>>>
>>
>> This was former version. Laurent pointed out that MIN macro
>> is using ternary operator which is expected to find the same time
>> on second and third part so the cast inside the MIN macro.
>> The cast before MIN was kept from previous code.
> 
> The C rules for ternary type promotion guarantee that the MIN macro
> produces the correct type without the cast ('cond ? int64_t : int32_t'
> produces int64_t). 

gdb seems to disagree with that:

(gdb) whatis l
type = long long
(gdb) whatis i
type = int
(gdb) whatis 1 ? l : i
type = long long
(gdb) whatis 0 ? l : i
type = int

It's on what I based my previous comment.

But both approaches are correct in the end.

Thanks,
Laurent