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). > + } > + 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> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature