On 2017-07-05 18:22, Max Reitz wrote: > On 2017-07-05 18:05, Max Reitz wrote: >> On 2017-07-05 15:48, Max Reitz wrote: >>> On 2017-07-05 09:07, Markus Armbruster wrote: >>>> Max Reitz <[email protected]> writes: >>>> >>>>> This generic function (along with its implementations for different >>>>> types) determines whether two QObjects are equal. >>>>> >>>>> Signed-off-by: Max Reitz <[email protected]> >>>> [...] >>>>> 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. :-) >> >> On second thought, this won't do, because (double)x == x is always true >> if x is an integer (because this will implicitly cast the second x to a >> double, too). However, (uint64_t)(double)x == x should work. > > Hm, well, the nice thing with this is that (double)UINT64_MAX is > actually UINT64_MAX + 1, and now (uint64_t)(UINT64_MAX + 1) is > undefined... Urgs. > > So I guess one thing that isn't very obvious but that should *always* > work (and is always well-defined) is this: > > For uint64_t: y < 0x1p64 && (uint64_t)y == x
Here comes iteration number 4: Forgot the y >= 0 check. > For int64_t: y >= -0x1p63 && y < 0x1p63 && (int64_t)y == x Also, I should check that the fractional part of y is 0 (through modf(y, &_) == 0.0). Floating point numbers are so much fun! (And all of this gives me such great ideas for tests to add to patch 5!) Max > I hope. :-/ > > (But finally a chance to use binary exponents! Yay!) > > Max >
signature.asc
Description: OpenPGP digital signature
