On Tue, Nov 17, 2020 at 7:49 PM Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote:
> > On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> >
> > > Extract the QNum value comparison logic to a function that takes
> > > QNumValue* as argument.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  include/qapi/qmp/qnum.h |  1 +
> > >  qobject/qnum.c          | 29 +++++++++++++++++++----------
> > >  2 files changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > > index 62fbdfda68..0327ecd0f0 100644
> > > --- a/include/qapi/qmp/qnum.h
> > > +++ b/include/qapi/qmp/qnum.h
> > > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
> > >
> > >  char *qnum_to_string(QNum *qn);
> > >
> > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue
> *num_y);
> > >  bool qnum_is_equal(const QObject *x, const QObject *y);
> > >  void qnum_destroy_obj(QObject *obj);
> > >
> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > > index f80d4efd76..6a0f948b16 100644
> > > --- a/qobject/qnum.c
> > > +++ b/qobject/qnum.c
> > > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> > >  }
> > >
> > >  /**
> > > - * qnum_is_equal(): Test whether the two QNums are equal
> > > - * @x: QNum object
> > > - * @y: QNum object
> > > + * qnum_value_is_equal(): Test whether two QNumValues are equal
> > > + * @num_x: QNum value
> > > + * @num_y: QNum value
> > >
> >
> > val_x: a QNumValue ?
>
> Do you mean:
>   @num_x: a QNumValue
>
?
>
> I was not planning to rename the existing num_x/num_y variables.
>
>
Not renaming because that would make some churn? But this is already quite
confusing, so it's better to use "val" for QNumVal and "num" for QNum I
guess.

If you don't want to rename it in the code, may I suggest doing it at the
declaration side? Not sure it's a better idea.


> >
> >   *
> > >   * Negative integers are never considered equal to unsigned integers,
> > >   * but positive integers in the range [0, INT64_MAX] are considered
> > > @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> > >   *
> > >   * Doubles are never considered equal to integers.
> > >   */
> > > -bool qnum_is_equal(const QObject *x, const QObject *y)
> > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue
> *num_y)
> > >  {
> > > -    const QNum *qnum_x = qobject_to(QNum, x);
> > > -    const QNum *qnum_y = qobject_to(QNum, y);
> > > -    const QNumValue *num_x = &qnum_x->value;
> > > -    const QNumValue *num_y = &qnum_y->value;
> > > -
> > >      switch (num_x->kind) {
> > >      case QNUM_I64:
> > >          switch (num_y->kind) {
> > > @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject
> *y)
> > >      case QNUM_U64:
> > >          switch (num_y->kind) {
> > >          case QNUM_I64:
> > > -            return qnum_is_equal(y, x);
> > > +            return qnum_value_is_equal(num_y, num_x);
> > >          case QNUM_U64:
> > >              /* Comparison in native uint64_t type */
> > >              return num_x->u.u64 == num_y->u.u64;
> > > @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const
> QObject *y)
> > >      abort();
> > >  }
> > >
> > > +/**
> > > + * qnum_is_equal(): Test whether the two QNums are equal
> > > + * @x: QNum object
> > > + * @y: QNum object
> > > + *
> > > + * See qnum_value_is_equal() for details on the comparison rules.
> > > + */
> > > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > > +{
> > > +    const QNum *qnum_x = qobject_to(QNum, x);
> > > +    const QNum *qnum_y = qobject_to(QNum, y);
> > > +    return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > > +}
> > > +
> > >  /**
> > >   * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > >   *
> > > --
> > > 2.28.0
> > >
> > >
> > >
> > beside that,
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Thanks!
>
> --
> Eduardo
>
>

-- 
Marc-André Lureau