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

[Max Reitz]

On 2017-07-05 21:49, Eric Blake wrote:
> 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).

Infinity is covered by the range check, though.

Max

> 
>> +        }
>> +        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>
>

signature.asc
Description: OpenPGP digital signature