RE: [PATCH 02/19] host-utils: move abs64() to host-utils

[Luis Fernando Fujita Pires]

> >> Oh, I wasn't referring to any specific users. What I meant is that,
> >> if we make abs64() generically available from host-utils, callers
> >> could expect it to behave the same way as abs() in stdlib, for
> >> example.
> >
> > That would be surprising, but do you think there are cases where that
> > would be a bad surprise?
> >
> > I don't think anybody who is aware of the abs(INT_MIN),
> > labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_ that
> > behaviour.
> >
> > If you really want to avoid surprises, providing a saner function with
> > a different name seems better than trying to emulate the edge cases of
> > abs()/labs()/llabs().
> 
> Agreed. See do_strtosz() for example.

I'll make this change when I submit the next version of this patch series.

Thanks!

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH 02/19] host-utils: move abs64() to host-utils

[Philippe Mathieu-Daudé]

On 8/25/21 11:18 PM, Eduardo Habkost wrote:
> On Wed, Aug 25, 2021 at 08:37:17PM +, Luis Fernando Fujita Pires wrote:
>> From: Eduardo Habkost 
>>
 Right, that's true of any standard implementation of abs().
 I thought about making it return uint64_t, but that could make it
 weird for other uses of abs64(), where callers wouldn't expect a type
 change from int64_t to uint64_t. Maybe create a separate uabs64() that
 returns uint64_t? Or is that even weirder? :)
>>>
>>> Which users of abs64 would expect it to return int64_t?
>>> kvm_pit_update_clock_offset() doesn't seem to.
>>
>> Oh, I wasn't referring to any specific users. What I meant is
>> that, if we make abs64() generically available from host-utils,
>> callers could expect it to behave the same way as abs() in
>> stdlib, for example.
> 
> That would be surprising, but do you think there are cases where
> that would be a bad surprise?
> 
> I don't think anybody who is aware of the abs(INT_MIN),
> labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_
> that behaviour.
> 
> If you really want to avoid surprises, providing a saner function
> with a different name seems better than trying to emulate the
> edge cases of abs()/labs()/llabs().

Agreed. See do_strtosz() for example.

Re: [PATCH 02/19] host-utils: move abs64() to host-utils

[Eduardo Habkost]

On Wed, Aug 25, 2021 at 08:37:17PM +, Luis Fernando Fujita Pires wrote:
> From: Eduardo Habkost 
> 
> > > Right, that's true of any standard implementation of abs().
> > > I thought about making it return uint64_t, but that could make it
> > > weird for other uses of abs64(), where callers wouldn't expect a type
> > > change from int64_t to uint64_t. Maybe create a separate uabs64() that
> > > returns uint64_t? Or is that even weirder? :)
> > 
> > Which users of abs64 would expect it to return int64_t?
> > kvm_pit_update_clock_offset() doesn't seem to.
> 
> Oh, I wasn't referring to any specific users. What I meant is
> that, if we make abs64() generically available from host-utils,
> callers could expect it to behave the same way as abs() in
> stdlib, for example.

That would be surprising, but do you think there are cases where
that would be a bad surprise?

I don't think anybody who is aware of the abs(INT_MIN),
labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_
that behaviour.

If you really want to avoid surprises, providing a saner function
with a different name seems better than trying to emulate the
edge cases of abs()/labs()/llabs().

-- 
Eduardo

RE: [PATCH 02/19] host-utils: move abs64() to host-utils

[Luis Fernando Fujita Pires]

From: Eduardo Habkost 

> > Right, that's true of any standard implementation of abs().
> > I thought about making it return uint64_t, but that could make it
> > weird for other uses of abs64(), where callers wouldn't expect a type
> > change from int64_t to uint64_t. Maybe create a separate uabs64() that
> > returns uint64_t? Or is that even weirder? :)
> 
> Which users of abs64 would expect it to return int64_t?
> kvm_pit_update_clock_offset() doesn't seem to.

Oh, I wasn't referring to any specific users. What I meant is that, if we make 
abs64() generically available from host-utils, callers could expect it to 
behave the same way as abs() in stdlib, for example.

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH 02/19] host-utils: move abs64() to host-utils

[Eduardo Habkost]

On Wed, Aug 25, 2021 at 12:48:35PM +, Luis Fernando Fujita Pires wrote:
> From: David Gibson 
> > Hrm..  I'm a bit concerned about mkaing this a more widespread function,
> > because it has a nasty edge case... which is basically unavoidable in an 
> > abs64()
> > implementation.  Specifically:
> > 
> > abs64(0x800___0) == 0x800___ < 0
> > 
> > At least in the most likely 2's complement implementation.
> 
> Right, that's true of any standard implementation of abs().
> I thought about making it return uint64_t, but that could make
> it weird for other uses of abs64(), where callers wouldn't
> expect a type change from int64_t to uint64_t. Maybe create a
> separate uabs64() that returns uint64_t? Or is that even
> weirder? :)

Which users of abs64 would expect it to return int64_t?
kvm_pit_update_clock_offset() doesn't seem to.

-- 
Eduardo

RE: [PATCH 02/19] host-utils: move abs64() to host-utils

[Luis Fernando Fujita Pires]

From: David Gibson 
> Hrm..  I'm a bit concerned about mkaing this a more widespread function,
> because it has a nasty edge case... which is basically unavoidable in an 
> abs64()
> implementation.  Specifically:
> 
> abs64(0x800___0) == 0x800___ < 0
> 
> At least in the most likely 2's complement implementation.

Right, that's true of any standard implementation of abs().
I thought about making it return uint64_t, but that could make it weird for 
other uses of abs64(), where callers wouldn't expect a type change from int64_t 
to uint64_t. Maybe create a separate uabs64() that returns uint64_t? Or is that 
even weirder? :)

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

Re: [PATCH 02/19] host-utils: move abs64() to host-utils

[David Gibson]

On Tue, Aug 24, 2021 at 11:27:13AM -0300, Luis Pires wrote:
> Move abs64 to host-utils so it can be reused elsewhere.
> Also made it inline.
> 
> Signed-off-by: Luis Pires 
> ---
>  hw/i386/kvm/i8254.c   | 5 -
>  include/qemu/host-utils.h | 8 
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
> index fa68669e8a..761034743b 100644
> --- a/hw/i386/kvm/i8254.c
> +++ b/hw/i386/kvm/i8254.c
> @@ -59,11 +59,6 @@ struct KVMPITClass {
>  DeviceRealize parent_realize;
>  };
>  
> -static int64_t abs64(int64_t v)
> -{
> -return v < 0 ? -v : v;

Hrm..  I'm a bit concerned about mkaing this a more widespread
function, because it has a nasty edge case... which is basically
unavoidable in an abs64() implementation.  Specifically:

abs64(0x800___0) == 0x800___ < 0

At least in the most likely 2's complement implementation.

> -}
> -
>  static void kvm_pit_update_clock_offset(KVMPITState *s)
>  {
>  int64_t offset, clock_offset;
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 711b221704..5fec44a9c4 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -357,6 +357,14 @@ static inline uint64_t revbit64(uint64_t x)
>  #endif
>  }
>  
> +/**
> + * Return the absolute value of a 64-bit integer
> + */
> +static inline int64_t abs64(int64_t v)
> +{
> +return v < 0 ? -v : v;
> +}
> +
>  /**
>   * sadd32_overflow - addition with overflow indication
>   * @x, @y: addends

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature