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