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

[Max Reitz]

On 2017-07-05 09:07, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
> 
>> This generic function (along with its implementations for different
>> types) determines whether two QObjects are equal.
>>
>> Signed-off-by: Max Reitz <address@hidden>
> [...]
>> diff --git a/qobject/qnum.c b/qobject/qnum.c
>> index 476e81c..784d061 100644
>> --- a/qobject/qnum.c
>> +++ b/qobject/qnum.c
>> @@ -213,6 +213,59 @@ QNum *qobject_to_qnum(const QObject *obj)
>>  }
>>  
>>  /**
>> + * qnum_is_equal(): Test whether the two QNums are equal
>> + */
>> +bool qnum_is_equal(const QObject *x, const QObject *y)
>> +{
>> +    QNum *num_x = qobject_to_qnum(x);
>> +    QNum *num_y = qobject_to_qnum(y);
>> +
>> +    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:
>> +            /* Implicit conversion of x to double; no overflow
>> +             * possible */
>> +            return num_x->u.i64 == num_y->u.dbl;
> 
> Overflow is impossible, but loss of precision is possible:
> 
>     (double)9007199254740993ull == 9007199254740992.0
> 
> yields true.  Is this what we want?

I'd argue that yes, because the floating point value represents
basically all of the values which are "equal" to it.

But I don't have a string opinion. I guess the alternative would be to
convert the double to an integer instead and check for overflows before?

>> +        }
>> +        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:
>> +            /* Implicit conversion of x to double; no overflow
>> +             * possible */
>> +            return num_x->u.u64 == num_y->u.dbl;
> 
> Similar loss of precision.
> 
>> +        }
>> +        abort();
>> +    case QNUM_DOUBLE:
>> +        switch (num_y->kind) {
>> +        case QNUM_I64:
>> +            return qnum_is_equal(y, x);
>> +        case QNUM_U64:
>> +            return qnum_is_equal(y, x);
>> +        case QNUM_DOUBLE:
>> +            /* Comparison in native double type */
>> +            return num_x->u.dbl == num_y->u.dbl;
>> +        }
>> +        abort();
>> +    }
>> +
>> +    abort();
>> +}
> 
> I think there's more than one sane interpretations of "is equal",
> including:
> 
> * The mathematical numbers represented by @x and @y are equal.
> 
> * @x and @y have the same contents, i.e. same kind and u.
> 
> * @x and @y are the same object (listed for completeness; we don't need
>   a function to compare pointers).
> 
> Your patch implements yet another one.  Which one do we want, and why?

Mine is the first one, except that I think that a floating point value
does not represent a single number but just some number in a range.

> The second is easier to implement than the first.

It seems much less useful, though.

> If we really want the first, you need to fix the loss of precision bugs.

I'm not sure, but I don't mind either, so...

> I guess the obvious fix is
> 
>     return (double)x == x && x == y;

Yes, that would do, too; and spares me of having to think about how well
comparing an arbitrary double to UINT64_MAX actually works. :-)

> Note that this is what you do for mixed signedness: first check @x is
> exactly representable in @y's type, then compare in @y's type.
> 
> Regardless of which one we pick, the function comment needs to explain.

OK, will do.

Max

>> +
>> +/**
>>   * qnum_destroy_obj(): Free all memory allocated by a
>>   * QNum object
>>   */
> [...]
> 
> Remainder of the patch looks good to me.
>

signature.asc
Description: OpenPGP digital signature