[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal() |
Date: |
Wed, 5 Jul 2017 15:48:53 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
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
- [Qemu-block] [PATCH v3 0/5] block: Don't compare strings in bdrv_reopen_prepare(), Max Reitz, 2017/07/03
- [Qemu-block] [PATCH v3 1/5] qapi/qnull: Add own header, Max Reitz, 2017/07/03
- [Qemu-block] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/03
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Eric Blake, 2017/07/03
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Markus Armbruster, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Eric Blake, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Eric Blake, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Eric Blake, 2017/07/05
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] qapi: Add qobject_is_equal(), Max Reitz, 2017/07/05