Re: [Qemu-devel] [RFC v4 02/13] qapi: qobject_compare() helper

[Eduardo Habkost]

On Tue, Aug 15, 2017 at 11:16:57AM -0500, Eric Blake wrote:
> On 08/14/2017 04:57 PM, Eduardo Habkost wrote:
> > The helper function will be useful when writing support code to
> > deal with device slot information.
> > 
> > TODO: documentation is incomplete and unclear, needs to be
> > improved.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  include/qapi/util.h    | 39 +++++++++++++++++++++++++++++
> >  qapi/qapi-util.c       | 66 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/test-qapi-util.c | 53 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 158 insertions(+)
> > 
> 
> > +/**
> > + * qobject_compare:
> > + *
> > + * Compare the value of @a and @b.
> > + *
> > + * If @a and @b have the same type and the same value (see list
> > + * of supported types below), return 0.
> > + *
> > + * If @a and @b are both strings, return strcmp(a, b).
> > + *
> > + * If @a and @b are numbers, return a negative value if a < b,
> > + * and a positive value if a > b.
> > + *
> > + * Otherwise (if @a and @b are not the same, have different types,
> > + * are of an unsupported type, or are different), return a non-zero value.
> 
> Is this number going to be commutative and distributive, in order to
> provide stable qsort()ing?  That is, if comparing a and b gives a
> positive number, then comparing b and a should give a negative number;
> and if comparing a and b then b and c results in two positive numbers,
> then comparing a and c should also give a positive number.  It is
> unclear from the documentation whether you are able to make this
> guarantee; and without it, it is unsafe to use this comparator in places
> that require stability.

No, I don't make that guarantee when the types don't match or in
the case of unsupported types.  That's a limitation of this
implementation.

Guaranteeing it when types don't match should be easy.
Guaranteeing it in the case of QTYPE_QDICT looks a bit harder.
Probably it's easier to simply implement full dictionary
comparison.

> 
> > + *
> > + * Note that this function doesn't support some types, and may
> > + * return false if the types are unsupported, or if the types don't
> > + * match exactly.
> 
> How is a return of false (== 0, which also means equivalent) correct?

This is a documentation bug.  Leftover from a version that
returned a boolean value (true for equal, false for not equal) in
the past.

> 
> > + *
> > + * Supported types:
> > + * - QTYPE_QNULL
> > + * - QTYPE_QSTRING
> > + * - QTYPE_QBOOL
> > + * - QTYPE_QNUM (integers only)
> > + * - QTYPE_QLIST
> > + *
> > + * Unsupported (always return false):
> > + * - QTYPE_QNUM (non-integer values)
> > + * - QTYPE_QDICT
> > + *
> > + * TODO: rewrite documentation to be clearer.
> > + * TODO: support non-integer QTYPE_NUM values and QTYPE_QDICT.
> 
> There's another patch series pending for qobject_is_equal(); should
> these two patches share approaches or even code?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01134.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02459.html

I will take a look.  Thanks!

-- 
Eduardo