Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4998 bytes --]

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 <mreitz@redhat.com>
> ---
> 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 <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]