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 <mre...@redhat.com> >> --- >> 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 <ebl...@redhat.com> >
signature.asc
Description: OpenPGP digital signature