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. Max > >> 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
