| Message ID | 20191023122652.2999-3-fziglio@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] util/async: avoid useless cast | expand |
Le 23/10/2019 à 14:26, Frediano Ziglio a écrit : > Signed-off-by: Frediano Ziglio <fziglio@redhat.com> > --- > 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); > } > > > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
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 <fziglio@redhat.com> >> --- >> 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);
> > 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 <fziglio@redhat.com> > >> --- > >> 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. Frediano
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 <fziglio@redhat.com> >>>> --- >>>> 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). I agree that the (int) cast was pre-existing, but as long as we are cleaning up useless casts, we might as well clean up both.
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 <fziglio@redhat.com> >>>>> --- >>>>> 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
On 10/23/19 9:45 AM, Laurent Vivier wrote: >> 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 looks like you've found a gdb bug. C99 6.5.15 p5 states: "If both the second and third operands have arithmetic type, the result type that would be determined by the usual arithmetic conversions, were they applied to those two operands, is the type of the result." and the usual arithmetic conversion of 'long long OP int' is 'long long', per 6.3.1.8.
Le 23/10/2019 à 16:51, Eric Blake a écrit : > On 10/23/19 9:45 AM, Laurent Vivier wrote: > >>> 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 looks like you've found a gdb bug. > > C99 6.5.15 p5 states: > "If both the second and third operands have arithmetic type, the result > type that would be determined by the usual arithmetic conversions, were > they applied to those two operands, is the type of the result." > > and the usual arithmetic conversion of 'long long OP int' is 'long > long', per 6.3.1.8. > Ok, you're right [1] Frediano, sorry for my misleading comment. Thanks, Laurent [1] and gcc agrees: int main(void) { long long l; int i; typeof(0 ? l : i) f; typeof(1 ? l : i) t; } (gdb) whatis l type = long long (gdb) whatis i type = int (gdb) whatis f type = long long (gdb) whatis t type = long long
Le 23/10/2019 à 14:26, Frediano Ziglio a écrit : > Signed-off-by: Frediano Ziglio <fziglio@redhat.com> > --- > 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); > } > > > Applied to my trivial-patches branch. I've updated the patch to remove the two useless casts. Eric, if you want to add your R-b, I can add it to the queued patch. Thanks, Laurent
On 10/24/19 12:31 PM, Laurent Vivier wrote: > Le 23/10/2019 à 14:26, Frediano Ziglio a écrit : >> Signed-off-by: Frediano Ziglio <fziglio@redhat.com> >> --- >> util/qemu-timer.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> > > Applied to my trivial-patches branch. > > I've updated the patch to remove the two useless casts. > > Eric, if you want to add your R-b, I can add it to the queued patch. I don't see it queued on https://github.com/vivier/qemu/branches yet, but if removing the two casts is the only difference from the original: Reviewed-by: Eric Blake <eblake@redhat.com>
Le 24/10/2019 à 20:03, Eric Blake a écrit : > On 10/24/19 12:31 PM, Laurent Vivier wrote: >> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit : >>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com> >>> --- >>> util/qemu-timer.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> > >>> >> >> Applied to my trivial-patches branch. >> >> I've updated the patch to remove the two useless casts. >> >> Eric, if you want to add your R-b, I can add it to the queued patch. > > I don't see it queued on https://github.com/vivier/qemu/branches yet, Sorry, forgot to push it. > but if removing the two casts is the only difference from the original: > > Reviewed-by: Eric Blake <eblake@redhat.com> > Added and pushed (branch trivial-patches), you can check. Thanks, Laurent
On 10/24/19 1:11 PM, Laurent Vivier wrote: > Le 24/10/2019 à 20:03, Eric Blake a écrit : >> On 10/24/19 12:31 PM, Laurent Vivier wrote: >>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit : >>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com> >>>> --- >>>> util/qemu-timer.c | 6 +----- >>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>> >> >>>> >>> >>> Applied to my trivial-patches branch. >>> >>> I've updated the patch to remove the two useless casts. >>> >>> Eric, if you want to add your R-b, I can add it to the queued patch. >> >> I don't see it queued on https://github.com/vivier/qemu/branches yet, > > Sorry, forgot to push it. > >> but if removing the two casts is the only difference from the original: >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> > > Added and pushed (branch trivial-patches), you can check. Looks good.
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); }
Signed-off-by: Frediano Ziglio <fziglio@redhat.com> --- util/qemu-timer.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)