[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 2/5] qapi: Add qobject_is_equal()
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v4 2/5] qapi: Add qobject_is_equal() |
Date: |
Wed, 5 Jul 2017 14:49:23 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
On 07/05/2017 02:04 PM, Max Reitz wrote:
> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> Markus also proposed just reporting two values as unequal if they have a
> different internal representation (i.e. a different QNum kind).
>
> I don't like this very much, because I feel like QInt and QFloat have
> been unified for a reason: Outside of these classes, nobody should care
> about the exact internal representation. In JSON, there is no
> difference anyway. We probably want to use integers as long as we can
> and doubles whenever we cannot.
>
> In any case, I feel like the class should hide the different internal
> representations from the user. This necessitates being able to compare
> floating point values against integers. Since apparently the main use
> of QObject is to parse and emit JSON (and represent such objects
> internally), we also have to agree that JSON doesn't make a difference:
> 42 is just the same as 42.0.
>
> Finally, I think it's rather pointless not to consider 42u and 42 the
> same value. But since unsigned/signed are two different kinds of QNums
> already, we cannot consider them equal without considering 42.0 equal,
> too.
>
> Because of this, I have decided to continue to compare QNum values even
> if they are of a different kind.
This explanation may deserve to be in the commit log proper.
> /**
> + * qnum_is_equal(): Test whether the two QNums are equal
> + *
> + * Negative integers are never considered equal to unsigned integers.
> + * Doubles are only considered equal to integers if their fractional
> + * part is zero and their integral part is exactly equal to the
> + * integer. Because doubles have limited precision, there are
> + * therefore integers which do not have an equal double (e.g.
> + * INT64_MAX).
> + */
> +bool qnum_is_equal(const QObject *x, const QObject *y)
> +{
> + QNum *num_x = qobject_to_qnum(x);
> + QNum *num_y = qobject_to_qnum(y);
> + double integral_part; /* Needed for the modf() calls below */
> +
> + switch (num_x->kind) {
> + case QNUM_I64:
> + switch (num_y->kind) {
> + case QNUM_I64:
> + /* Comparison in native int64_t type */
> + return num_x->u.i64 == num_y->u.i64;
> + case QNUM_U64:
> + /* Implicit conversion of x to uin64_t, so we have to
> + * check its sign before */
> + return num_x->u.i64 >= 0 && num_x->u.i64 == num_y->u.u64;
> + case QNUM_DOUBLE:
> + /* Comparing x to y in double (which the implicit
> + * conversion would do) is not exact. So after having
> + * checked that y is an integer in the int64_t range
> + * (i.e. that it is within bounds and its fractional part
> + * is zero), compare both as integers. */
> + return num_y->u.dbl >= -0x1p63 && num_y->u.dbl < 0x1p63 &&
> + modf(num_y->u.dbl, &integral_part) == 0.0 &&
'man modf': given modf(x, &iptr), if x is a NaN, a Nan is returned
(good, NaN, is never equal to any integer value). But if x is positive
infinity, +0 is returned...
> + num_x->u.i64 == (int64_t)num_y->u.dbl;
...and *iptr is set to positive infinity. You are now converting
infinity to int64_t (whether via num_y->u.dbl or via &integral_part),
which falls in the unspecified portion of C99 (your quotes from 6.3.1.4
mentioned converting a finite value of real to integer, and say nothing
about converting NaN or infinity to integer).
Adding an 'isfinite(num_y->u.dbl) &&' to the expression would cover your
bases (or even 'isfinite(integral_part)', if we are worried about a
static checker complaining that we assign but never read integral_part).
> + }
> + abort();
> + case QNUM_U64:
> + switch (num_y->kind) {
> + case QNUM_I64:
> + return qnum_is_equal(y, x);
> + case QNUM_U64:
> + /* Comparison in native uint64_t type */
> + return num_x->u.u64 == num_y->u.u64;
> + case QNUM_DOUBLE:
> + /* Comparing x to y in double (which the implicit
> + * conversion would do) is not exact. So after having
> + * checked that y is an integer in the uint64_t range
> + * (i.e. that it is within bounds and its fractional part
> + * is zero), compare both as integers. */
> + return num_y->u.dbl >= 0 && num_y->u.dbl < 0x1p64 &&
> + modf(num_y->u.dbl, &integral_part) == 0.0 &&
> + num_x->u.u64 == (uint64_t)num_y->u.dbl;
And again.
With that addition,
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v4 0/5] block: Don't compare strings in bdrv_reopen_prepare(), Max Reitz, 2017/07/05
- [Qemu-block] [PATCH v4 1/5] qapi/qnull: Add own header, Max Reitz, 2017/07/05
- [Qemu-block] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05
- Re: [Qemu-block] [PATCH v4 2/5] qapi: Add qobject_is_equal(),
Eric Blake <=
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Markus Armbruster, 2017/07/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/09
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Markus Armbruster, 2017/07/10
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/10
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Markus Armbruster, 2017/07/11
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/11
- [Qemu-block] [PATCH v4 3/5] block: qobject_is_equal() in bdrv_reopen_prepare(), Max Reitz, 2017/07/05
- [Qemu-block] [PATCH v4 4/5] iotests: Add test for non-string option reopening, Max Reitz, 2017/07/05