Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

On 2017-08-14 11:07, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 2017-07-11 13:33, Markus Armbruster wrote:
>>> Max Reitz  writes:
>>>
 First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
 least not right now and in the foreseeable future).
 You're the maintainer, so you decide, so I'll go along with it. :-)

 Now, let's follow up with my therefore rather useless commentary:

 (Feel free to disregard, because honestly, I can see how replying to
 most of the points I'm asking isn't really worth the time...)
>>>
>>> When I use the authority entrusted to maintainers, I feel obliged to at
>>> least explain my reasoning.  Besides, putting my reasoning in words
>>> tends to lead me to new insights.
>>
>> And I am indeed very grateful for that. :-)
>>
 On 2017-07-10 11:17, Markus Armbruster wrote:
> Max Reitz  writes:
>
>> On 2017-07-06 16:30, Markus Armbruster wrote:
>>
>> [...]
>>
> The only way to add unsigned integers without breaking QMP compatibility
> is to make them interchangeable with signed integers.  That doesn't mean
> you get to make floating-point numbers interchangeable with integers
> now.

 Again, begs the question why QNum covers floating point numbers then and
 why this very fact is not documented in qnum.c.
>>>
>>> What kind of documentation would you like to see?
>>
>> It would be good to note that the QNum type is not meant to be a
>> completely uniform way to handle JSON numbers (e.g. if the user provides
>> something with a decimal point but you need an integer, QNum will not do
>> that conversion for you).
>>
>> It is (English indirect speech is broken badly) just meant to
>> encapsulate the different variants a number can be represented in, but
>> you're still generally supposed to read it out the way it was put in
>> (exceptions apply, see signed/unsigned and qnum_get_double()).
> 
> Can we distill this into text that could become an actual patch?  Let me
> try.
> 
> QNum encapsulates how our dialect of JSON fills in the blanks left
> by the JSON specification (RFC 7159) regarding numbers.
> 
> Conceptually, we treat number as an abstract type with three
> concrete subtypes: floating-point, signed integer, unsigned integer.
> QNum implements this a discriminated union of double, int64_t,
> uint64_t.
> 
> The JSON parser picks the subtype as follows.  If the number has a
> decimal point or an exponent, it is floating-point.  Else if it fits
> into int64_t, it's signed integer.  Else if it first into uint64_t,
> it's unsigned integer.  Else it's floating-point.
> 
> Any number can serve as double: qnum_get_double() converts under the
> hood.
> 
> An integer can serve as signed / unsigned integer as long as it is
> in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> convert under the hood.
> 
> What do you think?

Sounds very good to me, thanks!

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz  writes:

> On 2017-07-11 13:33, Markus Armbruster wrote:
>> Max Reitz  writes:
>> 
>>> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
>>> least not right now and in the foreseeable future).
>>> You're the maintainer, so you decide, so I'll go along with it. :-)
>>>
>>> Now, let's follow up with my therefore rather useless commentary:
>>>
>>> (Feel free to disregard, because honestly, I can see how replying to
>>> most of the points I'm asking isn't really worth the time...)
>> 
>> When I use the authority entrusted to maintainers, I feel obliged to at
>> least explain my reasoning.  Besides, putting my reasoning in words
>> tends to lead me to new insights.
>
> And I am indeed very grateful for that. :-)
>
>>> On 2017-07-10 11:17, Markus Armbruster wrote:
 Max Reitz  writes:

> On 2017-07-06 16:30, Markus Armbruster wrote:
>
> [...]
>
 The only way to add unsigned integers without breaking QMP compatibility
 is to make them interchangeable with signed integers.  That doesn't mean
 you get to make floating-point numbers interchangeable with integers
 now.
>>>
>>> Again, begs the question why QNum covers floating point numbers then and
>>> why this very fact is not documented in qnum.c.
>> 
>> What kind of documentation would you like to see?
>
> It would be good to note that the QNum type is not meant to be a
> completely uniform way to handle JSON numbers (e.g. if the user provides
> something with a decimal point but you need an integer, QNum will not do
> that conversion for you).
>
> It is (English indirect speech is broken badly) just meant to
> encapsulate the different variants a number can be represented in, but
> you're still generally supposed to read it out the way it was put in
> (exceptions apply, see signed/unsigned and qnum_get_double()).

Can we distill this into text that could become an actual patch?  Let me
try.

QNum encapsulates how our dialect of JSON fills in the blanks left
by the JSON specification (RFC 7159) regarding numbers.

Conceptually, we treat number as an abstract type with three
concrete subtypes: floating-point, signed integer, unsigned integer.
QNum implements this a discriminated union of double, int64_t,
uint64_t.

The JSON parser picks the subtype as follows.  If the number has a
decimal point or an exponent, it is floating-point.  Else if it fits
into int64_t, it's signed integer.  Else if it first into uint64_t,
it's unsigned integer.  Else it's floating-point.

Any number can serve as double: qnum_get_double() converts under the
hood.

An integer can serve as signed / unsigned integer as long as it is
in range: qnum_get_try_int() / qnum_get_try_uint() check range and
convert under the hood.

What do you think?

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

On 2017-07-11 13:33, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
>> least not right now and in the foreseeable future).
>> You're the maintainer, so you decide, so I'll go along with it. :-)
>>
>> Now, let's follow up with my therefore rather useless commentary:
>>
>> (Feel free to disregard, because honestly, I can see how replying to
>> most of the points I'm asking isn't really worth the time...)
> 
> When I use the authority entrusted to maintainers, I feel obliged to at
> least explain my reasoning.  Besides, putting my reasoning in words
> tends to lead me to new insights.

And I am indeed very grateful for that. :-)

>> On 2017-07-10 11:17, Markus Armbruster wrote:
>>> Max Reitz  writes:
>>>
 On 2017-07-06 16:30, Markus Armbruster wrote:

[...]

>>> The only way to add unsigned integers without breaking QMP compatibility
>>> is to make them interchangeable with signed integers.  That doesn't mean
>>> you get to make floating-point numbers interchangeable with integers
>>> now.
>>
>> Again, begs the question why QNum covers floating point numbers then and
>> why this very fact is not documented in qnum.c.
> 
> What kind of documentation would you like to see?

It would be good to note that the QNum type is not meant to be a
completely uniform way to handle JSON numbers (e.g. if the user provides
something with a decimal point but you need an integer, QNum will not do
that conversion for you).

It is (English indirect speech is broken badly) just meant to
encapsulate the different variants a number can be represented in, but
you're still generally supposed to read it out the way it was put in
(exceptions apply, see signed/unsigned and qnum_get_double()).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz  writes:

> First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
> least not right now and in the foreseeable future).
> You're the maintainer, so you decide, so I'll go along with it. :-)
>
> Now, let's follow up with my therefore rather useless commentary:
>
> (Feel free to disregard, because honestly, I can see how replying to
> most of the points I'm asking isn't really worth the time...)

When I use the authority entrusted to maintainers, I feel obliged to at
least explain my reasoning.  Besides, putting my reasoning in words
tends to lead me to new insights.

> On 2017-07-10 11:17, Markus Armbruster wrote:
>> Max Reitz  writes:
>> 
>>> On 2017-07-06 16:30, Markus Armbruster wrote:
 Max Reitz  writes:

> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
>
> Signed-off-by: Max Reitz 
> ---
> 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.

 You're right in that JSON has no notion of integer and floating-point,
 only "number".  RFC 4627 is famously useless[1] on what exactly a number
 ought to be, and its successor RFC 7159 could then (due to wildly
 varying existing practice) merely state that a number is what the
 implementation makes it to be, and advises "good interoperability can be
 achieved" by making it double".  Pffft.

 For us, being able to represent 64 bit integers is more important than
 interoperating with crappy JSON implementations, so we made it the union
 of int64_t, uint64_t and double[2].

 You make a fair point when you say that nothing outside QNum should care
 about the exact internal representation.  Trouble is that unless I'm
 mistaken, your idea of "care" doesn't match the existing code's idea.
>>>
>>> I disagree that it doesn't match the existing code's idea.  I think the
>>> existing code doesn't match its idea, but mine does.
>>>
 Let i42 = qnum_from_int(42)
 u42 = qnum_from_uint(42)
 d42 = qnum_from_double(42)

 Then

 qnum_is_equal(i42, u42) yields true, I think.
 qnum_is_equal(i42, d42) yields true, I think.
 qnum_get_int(i42) yields 42.
 qnum_get_int(u42) yields 42.
 qnum_get_int(d42) fails its assertion.

 Failing an assertion qualifies as "care", doesn't it?
>>>
>>> It doesn't convert the value?  That's definitely not what I would have
>>> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)
>> 
>> It's the existing code's idea, going back all the way to the dawn of
>> QMP: integers and floating point numbers are distinct.
>> 
>> Yes, they aren't distinct in the JSON grammar.  So sue the designers of
>> QMP.
>
> Sounds like it was a reasonable idea at the time but could be done
> better today.  But that's how it always is, right?

Finding fault with designs we've inherited turns out to be much easier
than coming up with designs that last.

>> Yes, they are less distinct in QMP than say integers and strings,
>> because there's an automatic conversion from integer to floating point.
>> Doesn't make them non-distinct; there is no conversion from floating
>> point to integer.
>
> I can very well see that as a technical reason, but OK.
>
>> Yes, we recently changed the code to use the same C type for both.  That
>> was done to keep the code simple, not to change the semantics of QMP.
>
> Hm, OK.
>
>>> From the other side we see that qnum_get_double() on all of this would
>>> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
>>> Because there are doubles you cannot reasonably convert to integers, I
>>> presume, whereas the other way around the worst that can happen is that
>>> you lose some precision.
>>>
>>> But that has no implication on qnum_is_equal().  If the double cannot be
>>> converted to an integer because it is out of bounds, the values just are
>>> not equal.  Simple.
>>>
>>> So since qnum_get_double() does a conversion, I very much think that the
>>> reason qnum_get_int() doesn't is mostly "because sometimes it's not
>>> reasonably possible" and very much not because it is not intended to.
>> 
>> It doesn't because the whole shebang is for QMP, and QMP does not ever
>> treat floating point numbers (numbers with decimal point or exponent) as
>> integers.
>
> Well, to my defense, I couldn't see that from looking at the code.  From
> that point of 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

First of all, OK, you don't want QNum(42.0) to equal QNum(42) at all (at
least not right now and in the foreseeable future).
You're the maintainer, so you decide, so I'll go along with it. :-)

Now, let's follow up with my therefore rather useless commentary:

(Feel free to disregard, because honestly, I can see how replying to
most of the points I'm asking isn't really worth the time...)

On 2017-07-10 11:17, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 2017-07-06 16:30, Markus Armbruster wrote:
>>> Max Reitz  writes:
>>>
 This generic function (along with its implementations for different
 types) determines whether two QObjects are equal.

 Signed-off-by: Max Reitz 
 ---
 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.
>>>
>>> You're right in that JSON has no notion of integer and floating-point,
>>> only "number".  RFC 4627 is famously useless[1] on what exactly a number
>>> ought to be, and its successor RFC 7159 could then (due to wildly
>>> varying existing practice) merely state that a number is what the
>>> implementation makes it to be, and advises "good interoperability can be
>>> achieved" by making it double".  Pffft.
>>>
>>> For us, being able to represent 64 bit integers is more important than
>>> interoperating with crappy JSON implementations, so we made it the union
>>> of int64_t, uint64_t and double[2].
>>>
>>> You make a fair point when you say that nothing outside QNum should care
>>> about the exact internal representation.  Trouble is that unless I'm
>>> mistaken, your idea of "care" doesn't match the existing code's idea.
>>
>> I disagree that it doesn't match the existing code's idea.  I think the
>> existing code doesn't match its idea, but mine does.
>>
>>> Let i42 = qnum_from_int(42)
>>> u42 = qnum_from_uint(42)
>>> d42 = qnum_from_double(42)
>>>
>>> Then
>>>
>>> qnum_is_equal(i42, u42) yields true, I think.
>>> qnum_is_equal(i42, d42) yields true, I think.
>>> qnum_get_int(i42) yields 42.
>>> qnum_get_int(u42) yields 42.
>>> qnum_get_int(d42) fails its assertion.
>>>
>>> Failing an assertion qualifies as "care", doesn't it?
>>
>> It doesn't convert the value?  That's definitely not what I would have
>> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)
> 
> It's the existing code's idea, going back all the way to the dawn of
> QMP: integers and floating point numbers are distinct.
> 
> Yes, they aren't distinct in the JSON grammar.  So sue the designers of
> QMP.

Sounds like it was a reasonable idea at the time but could be done
better today.  But that's how it always is, right?

> Yes, they are less distinct in QMP than say integers and strings,
> because there's an automatic conversion from integer to floating point.
> Doesn't make them non-distinct; there is no conversion from floating
> point to integer.

I can very well see that as a technical reason, but OK.

> Yes, we recently changed the code to use the same C type for both.  That
> was done to keep the code simple, not to change the semantics of QMP.

Hm, OK.

>> From the other side we see that qnum_get_double() on all of this would
>> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
>> Because there are doubles you cannot reasonably convert to integers, I
>> presume, whereas the other way around the worst that can happen is that
>> you lose some precision.
>>
>> But that has no implication on qnum_is_equal().  If the double cannot be
>> converted to an integer because it is out of bounds, the values just are
>> not equal.  Simple.
>>
>> So since qnum_get_double() does a conversion, I very much think that the
>> reason qnum_get_int() doesn't is mostly "because sometimes it's not
>> reasonably possible" and very much not because it is not intended to.
> 
> It doesn't because the whole shebang is for QMP, and QMP does not ever
> treat floating point numbers (numbers with decimal point or exponent) as
> integers.

Well, to my defense, I couldn't see that from looking at the code.  From
that point of view, it just looks like qnum_get_int() is lacking.

> Yes, there are users other than QMP.  They adopted it because it was
> convenient.  They thus adopted its oddities due to QMP's requirements,
> too.

To me, that mostly sounds like an excuse that distinguishing between
integers and floats will not be wrong, but not like a reason it is right.

 In any case, I feel like the class should hide the different internal
 representations 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz  writes:

> On 2017-07-06 16:30, Markus Armbruster wrote:
>> Max Reitz  writes:
>> 
>>> This generic function (along with its implementations for different
>>> types) determines whether two QObjects are equal.
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>> 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.
>> 
>> You're right in that JSON has no notion of integer and floating-point,
>> only "number".  RFC 4627 is famously useless[1] on what exactly a number
>> ought to be, and its successor RFC 7159 could then (due to wildly
>> varying existing practice) merely state that a number is what the
>> implementation makes it to be, and advises "good interoperability can be
>> achieved" by making it double".  Pffft.
>> 
>> For us, being able to represent 64 bit integers is more important than
>> interoperating with crappy JSON implementations, so we made it the union
>> of int64_t, uint64_t and double[2].
>> 
>> You make a fair point when you say that nothing outside QNum should care
>> about the exact internal representation.  Trouble is that unless I'm
>> mistaken, your idea of "care" doesn't match the existing code's idea.
>
> I disagree that it doesn't match the existing code's idea.  I think the
> existing code doesn't match its idea, but mine does.
>
>> Let i42 = qnum_from_int(42)
>> u42 = qnum_from_uint(42)
>> d42 = qnum_from_double(42)
>> 
>> Then
>> 
>> qnum_is_equal(i42, u42) yields true, I think.
>> qnum_is_equal(i42, d42) yields true, I think.
>> qnum_get_int(i42) yields 42.
>> qnum_get_int(u42) yields 42.
>> qnum_get_int(d42) fails its assertion.
>> 
>> Failing an assertion qualifies as "care", doesn't it?
>
> It doesn't convert the value?  That's definitely not what I would have
> thought and it doesn't make a lot of sense to me.  I call that a bug. :-)

It's the existing code's idea, going back all the way to the dawn of
QMP: integers and floating point numbers are distinct.

Yes, they aren't distinct in the JSON grammar.  So sue the designers of
QMP.

Yes, they are less distinct in QMP than say integers and strings,
because there's an automatic conversion from integer to floating point.
Doesn't make them non-distinct; there is no conversion from floating
point to integer.

Yes, we recently changed the code to use the same C type for both.  That
was done to keep the code simple, not to change the semantics of QMP.

> From the other side we see that qnum_get_double() on all of this would
> yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
> Because there are doubles you cannot reasonably convert to integers, I
> presume, whereas the other way around the worst that can happen is that
> you lose some precision.
>
> But that has no implication on qnum_is_equal().  If the double cannot be
> converted to an integer because it is out of bounds, the values just are
> not equal.  Simple.
>
> So since qnum_get_double() does a conversion, I very much think that the
> reason qnum_get_int() doesn't is mostly "because sometimes it's not
> reasonably possible" and very much not because it is not intended to.

It doesn't because the whole shebang is for QMP, and QMP does not ever
treat floating point numbers (numbers with decimal point or exponent) as
integers.

Yes, there are users other than QMP.  They adopted it because it was
convenient.  They thus adopted its oddities due to QMP's requirements,
too.

>>> 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.
>> 
>> The JSON RFC is mum on that.
>> 
>> In *our* implementation of JSON, 42 and 42.0 have always been very much
>> *not* the same.  Proof:
>> 
>> -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
>> <- {"return": {}}
>> -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
>> <- {"error": {"class": "GenericError", "desc": "Invalid parameter type 
>> for 'value', expected: integer"}}
>> 
>> This is because migrate_set_speed argument value is 'int', and 42.0 is
>> not a valid 'int' value.
>
> Well, that's a bug, too.  It's nice that we accept things that aren't
> quite valid JSON (I'm looking at you, single quote), but we should
> accept 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

On 2017-07-06 16:30, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> This generic function (along with its implementations for different
>> types) determines whether two QObjects are equal.
>>
>> Signed-off-by: Max Reitz 
>> ---
>> 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.
> 
> You're right in that JSON has no notion of integer and floating-point,
> only "number".  RFC 4627 is famously useless[1] on what exactly a number
> ought to be, and its successor RFC 7159 could then (due to wildly
> varying existing practice) merely state that a number is what the
> implementation makes it to be, and advises "good interoperability can be
> achieved" by making it double".  Pffft.
> 
> For us, being able to represent 64 bit integers is more important than
> interoperating with crappy JSON implementations, so we made it the union
> of int64_t, uint64_t and double[2].
> 
> You make a fair point when you say that nothing outside QNum should care
> about the exact internal representation.  Trouble is that unless I'm
> mistaken, your idea of "care" doesn't match the existing code's idea.

I disagree that it doesn't match the existing code's idea.  I think the
existing code doesn't match its idea, but mine does.

> Let i42 = qnum_from_int(42)
> u42 = qnum_from_uint(42)
> d42 = qnum_from_double(42)
> 
> Then
> 
> qnum_is_equal(i42, u42) yields true, I think.
> qnum_is_equal(i42, d42) yields true, I think.
> qnum_get_int(i42) yields 42.
> qnum_get_int(u42) yields 42.
> qnum_get_int(d42) fails its assertion.
> 
> Failing an assertion qualifies as "care", doesn't it?

It doesn't convert the value?  That's definitely not what I would have
thought and it doesn't make a lot of sense to me.  I call that a bug. :-)

From the other side we see that qnum_get_double() on all of this would
yield 42.0 without failing.  So why is it that qnum_get_int() doesn't?
Because there are doubles you cannot reasonably convert to integers, I
presume, whereas the other way around the worst that can happen is that
you lose some precision.

But that has no implication on qnum_is_equal().  If the double cannot be
converted to an integer because it is out of bounds, the values just are
not equal.  Simple.

So since qnum_get_double() does a conversion, I very much think that the
reason qnum_get_int() doesn't is mostly "because sometimes it's not
reasonably possible" and very much not because it is not intended to.

>> 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.
> 
> The JSON RFC is mum on that.
> 
> In *our* implementation of JSON, 42 and 42.0 have always been very much
> *not* the same.  Proof:
> 
> -> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
> <- {"return": {}}
> -> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
> <- {"error": {"class": "GenericError", "desc": "Invalid parameter type 
> for 'value', expected: integer"}}
> 
> This is because migrate_set_speed argument value is 'int', and 42.0 is
> not a valid 'int' value.

Well, that's a bug, too.  It's nice that we accept things that aren't
quite valid JSON (I'm looking at you, single quote), but we should
accept things that are valid JSON.

> Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
> value is 'number':
> 
> -> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
> <- {"return": {}}
> -> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
> <- {"return": {}}
> 
> Don't blame me for the parts of QMP I inherited :)

I sure don't.  But I am willing to start a discussion by calling that a
bug. ;-)

QNum has only been introduced recently.  Before, we had a hard split of
QInt and QFloat.  So I'm not surprised that we haven't fixed everything yet.

OTOH the introduction of QNum to me signals that we do want to fix this
eventually.

>> 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.
> 
> Non sequitur.
> 
>> Because of this, I have decided to continue to compare QNum 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

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 
>> ---
>> 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, _part) == 0.0 &&
> 
> 'man modf': given modf(x, ), 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 _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, _part) == 0.0 &&
>> +num_x->u.u64 == (uint64_t)num_y->u.dbl;
> 
> And again.
> 
> With that addition,
> Reviewed-by: Eric Blake 
> 




signature.asc
Description: OpenPGP digital 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Markus Armbruster]

Max Reitz  writes:

> This generic function (along with its implementations for different
> types) determines whether two QObjects are equal.
>
> Signed-off-by: Max Reitz 
> ---
> 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.

You're right in that JSON has no notion of integer and floating-point,
only "number".  RFC 4627 is famously useless[1] on what exactly a number
ought to be, and its successor RFC 7159 could then (due to wildly
varying existing practice) merely state that a number is what the
implementation makes it to be, and advises "good interoperability can be
achieved" by making it double".  Pffft.

For us, being able to represent 64 bit integers is more important than
interoperating with crappy JSON implementations, so we made it the union
of int64_t, uint64_t and double[2].

You make a fair point when you say that nothing outside QNum should care
about the exact internal representation.  Trouble is that unless I'm
mistaken, your idea of "care" doesn't match the existing code's idea.

Let i42 = qnum_from_int(42)
u42 = qnum_from_uint(42)
d42 = qnum_from_double(42)

Then

qnum_is_equal(i42, u42) yields true, I think.
qnum_is_equal(i42, d42) yields true, I think.
qnum_get_int(i42) yields 42.
qnum_get_int(u42) yields 42.
qnum_get_int(d42) fails its assertion.

Failing an assertion qualifies as "care", doesn't it?

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

The JSON RFC is mum on that.

In *our* implementation of JSON, 42 and 42.0 have always been very much
*not* the same.  Proof:

-> { "execute": "migrate_set_speed", "arguments": { "value": 42 } }
<- {"return": {}}
-> { "execute": "migrate_set_speed", "arguments": { "value": 42.0 } }
<- {"error": {"class": "GenericError", "desc": "Invalid parameter type for 
'value', expected: integer"}}

This is because migrate_set_speed argument value is 'int', and 42.0 is
not a valid 'int' value.

Note that 42 *is* a valid 'number' value.  migrate_set_downtime argument
value is 'number':

-> { "execute": "migrate_set_downtime", "arguments": { "value": 42 } }
<- {"return": {}}
-> { "execute": "migrate_set_downtime", "arguments": { "value": 42.0 } }
<- {"return": {}}

Don't blame me for the parts of QMP I inherited :)

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

Non sequitur.

> Because of this, I have decided to continue to compare QNum values even
> if they are of a different kind.

I think comparing signed and unsigned integer QNums is fair and
consistent with how the rest of our code works.

Comparing integer and floating QNums isn't.  It's also a can of worms.
Are you sure we *need* to open that can *now*?

Are you sure a simple, stupid eql-like comparison won't do *for now*?
YAGNI!


[1] Standard reply to criticism of JSON: could be worse, could be XML.

[2] Union of int64_t and double until recently, plus bugs that could be
abused to "tunnel" uint64_t values.  Some of the bugs have to remain for
backward compatibility.

Re: [Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Eric Blake]

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 
> ---
> 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, _part) == 0.0 &&

'man modf': given modf(x, ), 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 _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, _part) == 0.0 &&
> +num_x->u.u64 == (uint64_t)num_y->u.dbl;

And again.

With that addition,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v4 2/5] qapi: Add qobject_is_equal()

[Max Reitz]

This generic function (along with its implementations for different
types) determines whether two QObjects are equal.

Signed-off-by: Max Reitz 
---
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.
---
 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   |  2 ++
 include/qapi/qmp/qnum.h|  1 +
 include/qapi/qmp/qobject.h |  9 ++
 include/qapi/qmp/qstring.h |  1 +
 qobject/qbool.c|  8 +
 qobject/qdict.c| 29 ++
 qobject/qlist.c| 32 
 qobject/qnull.c|  9 ++
 qobject/qnum.c | 73 ++
 qobject/qobject.c  | 29 ++
 qobject/qstring.c  |  9 ++
 14 files changed, 205 insertions(+)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index a4c..f77ea86 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -24,6 +24,7 @@ typedef struct QBool {
 QBool *qbool_from_bool(bool value);
 bool qbool_get_bool(const QBool *qb);
 QBool *qobject_to_qbool(const QObject *obj);
+bool qbool_is_equal(const QObject *x, const QObject *y);
 void qbool_destroy_obj(QObject *obj);
 
 #endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 363e431..84f8ea7 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -42,6 +42,7 @@ void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
+bool qdict_is_equal(const QObject *x, const QObject *y);
 void qdict_iter(const QDict *qdict,
 void (*iter)(const char *key, QObject *obj, void *opaque),
 void *opaque);
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index c4b5fda..24e1e9f 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -58,6 +58,7 @@ QObject *qlist_peek(QList *qlist);
 int qlist_empty(const QList *qlist);
 size_t qlist_size(const QList *qlist);
 QList *qobject_to_qlist(const QObject *obj);
+bool qlist_is_equal(const QObject *x, const QObject *y);
 void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index 48edad4..f4fbcae 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -23,4 +23,6 @@ static inline QObject *qnull(void)
 return _;
 }
 
+bool qnull_is_equal(const QObject *x, const QObject *y);
+
 #endif /* QNULL_H */
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 09d745c..237d01b 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -48,6 +48,7 @@ double qnum_get_double(QNum *qn);
 char *qnum_to_string(QNum *qn);
 
 QNum *qobject_to_qnum(const QObject *obj);
+bool qnum_is_equal(const QObject *x, const QObject *y);
 void qnum_destroy_obj(QObject *obj);
 
 #endif /* QNUM_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index ef1d1a9..38ac688 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -68,6 +68,15 @@ static inline void qobject_incref(QObject *obj)
 }
 
 /**
+ * qobject_is_equal(): Return whether the two objects are equal.
+ *
+ * Any of the pointers may be NULL; return true if both are.  Always
+ * return false if only one is (therefore a QNull object is not
+ * considered equal to a NULL pointer).
+ */
+bool qobject_is_equal(const QObject *x, const QObject *y);
+
+/**
  * qobject_destroy(): Free resources used by the object
  */
 void qobject_destroy(QObject *obj);
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 10076b7..65c05a9 100644
--- a/include/qapi/qmp/qstring.h