diff mbox series

[v2,4/8] qnum: qnum_value_is_equal() function

Message ID 20201116224143.1284278-5-ehabkost@redhat.com
State New
Headers show
Series qom: Use qlit to represent property defaults | expand

Commit Message

Eduardo Habkost Nov. 16, 2020, 10:41 p.m. UTC
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(-)

Comments

Marc-André Lureau Nov. 17, 2020, 8:42 a.m. UTC | #1
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 ?

  *
>   * 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>
Eduardo Habkost Nov. 17, 2020, 3:49 p.m. UTC | #2
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.

> 
>   *
> >   * 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!
Marc-André Lureau Nov. 17, 2020, 4:53 p.m. UTC | #3
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
>
>
Eduardo Habkost Nov. 17, 2020, 5:21 p.m. UTC | #4
On Tue, Nov 17, 2020 at 08:53:19PM +0400, Marc-André Lureau wrote:
> 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.

Yeah, I was not renaming them just to avoid churn.

However, I'm already replacing `qn` variables with `qv` at patch
3/8.  Replacing num_x/num_y with val_x/val_y at qnum_is_equal()
(at patch 3/8 as well) wouldn't hurt too much.
Markus Armbruster Nov. 19, 2020, 10:27 a.m. UTC | #5
Eduardo Habkost <ehabkost@redhat.com> writes:

> 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
>   *
>   * 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);

Humor me: blank line between declarations and statements, please.

> +    return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> +}
> +
>  /**
>   * qnum_destroy_obj(): Free all memory allocated by a QNum object
>   *
Eduardo Habkost Nov. 19, 2020, 6:24 p.m. UTC | #6
On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
[...]
> > +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);
> 
> Humor me: blank line between declarations and statements, please.

I can do it.  But why do you prefer it that way?

> 
> > +    return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > +}
> > +
> >  /**
> >   * qnum_destroy_obj(): Free all memory allocated by a QNum object
> >   *
Markus Armbruster Nov. 20, 2020, 6:52 a.m. UTC | #7
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
> [...]
>> > +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);
>> 
>> Humor me: blank line between declarations and statements, please.
>
> I can do it.  But why do you prefer it that way?

Habit borne out of C lacking other visual cues to distinguish
declarations and statements.

Declaration or statement?  Tell me quick, don't analyze!

    mumble(*mutter)();

This "obviously" declares @mutter as pointer to function returning
mumble.

Except when @mumble isn't a typedef name, but a function taking one
argument and returning a function that takes no argument.  Then it
passes *mutter to mumble(), and calls its return value.

The whole point of coding style is to help readers along.  Two stylistic
conventions that can help here:

1. In a function call, no space between the expression denoting the
   called function and the (parenthesized) argument list.  Elsewhere,
   space.

   So, when the example above is indeed a declaration, write it as

        mumble (*mutter)();

   If it's function calls, write it as

        mumble(*mutter)();

2. Separate declarations from statements with a blank line.  Do not mix
   them.

[...]
Eduardo Habkost Nov. 20, 2020, 6:22 p.m. UTC | #8
On Fri, Nov 20, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
> > [...]
> >> > +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);
> >> 
> >> Humor me: blank line between declarations and statements, please.
> >
> > I can do it.  But why do you prefer it that way?
> 
> Habit borne out of C lacking other visual cues to distinguish
> declarations and statements.

Why is the distinction important, when many variable declarations
also include initializer expressions that can be as complex as
other statements?

(The qobject_to() calls above are an example).

> 
> Declaration or statement?  Tell me quick, don't analyze!
> 
>     mumble(*mutter)();
> 
> This "obviously" declares @mutter as pointer to function returning
> mumble.
> 
> Except when @mumble isn't a typedef name, but a function taking one
> argument and returning a function that takes no argument.  Then it
> passes *mutter to mumble(), and calls its return value.
> 
> The whole point of coding style is to help readers along.  Two stylistic
> conventions that can help here:
> 
> 1. In a function call, no space between the expression denoting the
>    called function and the (parenthesized) argument list.  Elsewhere,
>    space.
> 
>    So, when the example above is indeed a declaration, write it as
> 
>         mumble (*mutter)();
> 
>    If it's function calls, write it as
> 
>         mumble(*mutter)();

This makes lots of sense.  Starting with a word followed by space
is what makes declarations visually distinguishable.

> 
> 2. Separate declarations from statements with a blank line.  Do not mix
>    them.

I'm not sure about this one, and I'm actually glad it is not part
of CODING_STYLE.  :)

(I'll still follow your advice as maintainer of that piece of
code, of course)
Markus Armbruster Nov. 23, 2020, 8:17 a.m. UTC | #9
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Nov 20, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
>> > [...]
>> >> > +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);
>> >> 
>> >> Humor me: blank line between declarations and statements, please.
>> >
>> > I can do it.  But why do you prefer it that way?
>> 
>> Habit borne out of C lacking other visual cues to distinguish
>> declarations and statements.
>
> Why is the distinction important, when many variable declarations
> also include initializer expressions that can be as complex as
> other statements?
>
> (The qobject_to() calls above are an example).

We read left to right, and we're not good at backtracking.  The earlier
I know I'm reading a declaration, the better.

>> Declaration or statement?  Tell me quick, don't analyze!
>> 
>>     mumble(*mutter)();
>> 
>> This "obviously" declares @mutter as pointer to function returning
>> mumble.
>> 
>> Except when @mumble isn't a typedef name, but a function taking one
>> argument and returning a function that takes no argument.  Then it
>> passes *mutter to mumble(), and calls its return value.
>> 
>> The whole point of coding style is to help readers along.  Two stylistic
>> conventions that can help here:
>> 
>> 1. In a function call, no space between the expression denoting the
>>    called function and the (parenthesized) argument list.  Elsewhere,
>>    space.
>> 
>>    So, when the example above is indeed a declaration, write it as
>> 
>>         mumble (*mutter)();
>> 
>>    If it's function calls, write it as
>> 
>>         mumble(*mutter)();
>
> This makes lots of sense.  Starting with a word followed by space
> is what makes declarations visually distinguishable.

Declarations need not match that pattern.  Also, it's a rather subtle
cue.

>> 2. Separate declarations from statements with a blank line.  Do not mix
>>    them.
>
> I'm not sure about this one, and I'm actually glad it is not part
> of CODING_STYLE.  :)

That's why I ask to "humor me" :)

For what it's worth, the convention is common enough to be supported by
the traditional BSD indent program.

> (I'll still follow your advice as maintainer of that piece of
> code, of course)

Thanks!
diff mbox series

Patch

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