[v2,3/8] qnum: QNumValue type for QNum value literals

Provide a separate QNumValue type that can be used for QNum value
literals without the referencing counting and memory allocation
features provided by QObject.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Fix "make check" failure, by updating check-qnum unit test to
  use the new struct fields
---
 include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
 qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
 tests/check-qnum.c      | 14 ++++----
 3 files changed, 84 insertions(+), 48 deletions(-)

On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:

> Provide a separate QNumValue type that can be used for QNum value
> literals without the referencing counting and memory allocation
> features provided by QObject.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Fix "make check" failure, by updating check-qnum unit test to
>   use the new struct fields
> ---
>  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
>  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
>  tests/check-qnum.c      | 14 ++++----
>  3 files changed, 84 insertions(+), 48 deletions(-)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 55c27b1c24..62fbdfda68 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -46,20 +46,56 @@ typedef enum {
>   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>   * convert under the hood.
>   */
> -struct QNum {
> -    struct QObjectBase_ base;
> +
> +/**
> + * struct QNumValue: the value of a QNum
> + *
> + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> + */
> +typedef struct QNumValue {
> +    /* private: */
>      QNumKind kind;
>      union {
>          int64_t i64;
>          uint64_t u64;
>          double dbl;
>      } u;
> +} QNumValue;
> +
> +#define QNUM_VAL_INT(value) \
> +    { .kind = QNUM_I64, .u.i64 = value }
> +#define QNUM_VAL_UINT(value) \
> +    { .kind = QNUM_U64, .u.u64 = value }
> +#define QNUM_VAL_DOUBLE(value) \
> +    { .kind = QNUM_DOUBLE, .u.dbl = value }
> +
> +struct QNum {
> +    struct QObjectBase_ base;
> +    QNumValue value;
>  };
>
> +/**
> + * qnum_from_int(): Create a new QNum from a QNumValue
>

Copy-pasta qnum_from_int() -> qnum_from_value()

I wonder if there is a check for that kind of mistake, as it may be common.

+ * @value: QNumValue
> + *
> + * Return strong reference.
> + */
> +QNum *qnum_from_value(QNumValue value);
>
+
>  QNum *qnum_from_int(int64_t value);
>  QNum *qnum_from_uint(uint64_t value);
>  QNum *qnum_from_double(double value);
>
> +/**
> + * qnum_get_value(): Get QNumValue from QNum
> + * @qn: QNum object
> + */
> +static inline const QNumValue *qnum_get_value(const QNum *qn)
> +{
> +    return &qn->value;
> +}
> +
>

Nothing uses that function in this patch. Should be put into use.

 bool qnum_get_try_int(const QNum *qn, int64_t *val);
>  int64_t qnum_get_int(const QNum *qn);
>
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 69fd9a82d9..f80d4efd76 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -15,6 +15,15 @@
>  #include "qemu/osdep.h"
>  #include "qapi/qmp/qnum.h"
>
> +QNum *qnum_from_value(QNumValue value)
>

I wonder if it shouldn't be made "inline" in header too (if that can help
avoid argument copy).

+{
> +    QNum *qn = g_new(QNum, 1);
> +
> +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> +    qn->value = value;
> +    return qn;
> +}
> +
>  /**
>   * qnum_from_int(): Create a new QNum from an int64_t
>   * @value: int64_t value
> @@ -23,13 +32,7 @@
>   */
>  QNum *qnum_from_int(int64_t value)
>  {
> -    QNum *qn = g_new(QNum, 1);
> -
> -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> -    qn->kind = QNUM_I64;
> -    qn->u.i64 = value;
> -
> -    return qn;
> +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
>  }
>
>  /**
> @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
>   */
>  QNum *qnum_from_uint(uint64_t value)
>  {
> -    QNum *qn = g_new(QNum, 1);
> -
> -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> -    qn->kind = QNUM_U64;
> -    qn->u.u64 = value;
> -
> -    return qn;
> +    return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
>  }
>
>  /**
> @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
>   */
>  QNum *qnum_from_double(double value)
>  {
> -    QNum *qn = g_new(QNum, 1);
> -
> -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> -    qn->kind = QNUM_DOUBLE;
> -    qn->u.dbl = value;
> -
> -    return qn;
> +    return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
>  }
>
>  /**
> @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
>   */
>  bool qnum_get_try_int(const QNum *qn, int64_t *val)
>  {
> -    switch (qn->kind) {
> +    const QNumValue *qv = &qn->value;
> +
> +    switch (qv->kind) {
>      case QNUM_I64:
> -        *val = qn->u.i64;
> +        *val = qv->u.i64;
>          return true;
>      case QNUM_U64:
> -        if (qn->u.u64 > INT64_MAX) {
> +        if (qv->u.u64 > INT64_MAX) {
>              return false;
>          }
> -        *val = qn->u.u64;
> +        *val = qv->u.u64;
>          return true;
>      case QNUM_DOUBLE:
>          return false;
> @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
>   */
>  bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
>  {
> -    switch (qn->kind) {
> +    const QNumValue *qv = &qn->value;
> +
> +    switch (qv->kind) {
>      case QNUM_I64:
> -        if (qn->u.i64 < 0) {
> +        if (qv->u.i64 < 0) {
>              return false;
>          }
> -        *val = qn->u.i64;
> +        *val = qv->u.i64;
>          return true;
>      case QNUM_U64:
> -        *val = qn->u.u64;
> +        *val = qv->u.u64;
>          return true;
>      case QNUM_DOUBLE:
>          return false;
> @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
>   */
>  double qnum_get_double(const QNum *qn)
>  {
> -    switch (qn->kind) {
> +    const QNumValue *qv = &qn->value;
> +
> +    switch (qv->kind) {
>      case QNUM_I64:
> -        return qn->u.i64;
> +        return qv->u.i64;
>      case QNUM_U64:
> -        return qn->u.u64;
> +        return qv->u.u64;
>      case QNUM_DOUBLE:
> -        return qn->u.dbl;
> +        return qv->u.dbl;
>      }
>
>      assert(0);
> @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
>
>  char *qnum_to_string(QNum *qn)
>  {
> +    const QNumValue *qv = &qn->value;
>

qnum_get_value() ?

     char *buffer;
>      int len;
>
> -    switch (qn->kind) {
> +    switch (qv->kind) {
>      case QNUM_I64:
> -        return g_strdup_printf("%" PRId64, qn->u.i64);
> +        return g_strdup_printf("%" PRId64, qv->u.i64);
>      case QNUM_U64:
> -        return g_strdup_printf("%" PRIu64, qn->u.u64);
> +        return g_strdup_printf("%" PRIu64, qv->u.u64);
>      case QNUM_DOUBLE:
>          /* FIXME: snprintf() is locale dependent; but JSON requires
>           * numbers to be formatted as if in the C locale. Dependence
> @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
>           * rounding errors; we should be using DBL_DECIMAL_DIG (17),
>           * and only rounding to a shorter number if the result would
>           * still produce the same floating point value.  */
> -        buffer = g_strdup_printf("%f" , qn->u.dbl);
> +        buffer = g_strdup_printf("%f" , qv->u.dbl);
>          len = strlen(buffer);
>          while (len > 0 && buffer[len - 1] == '0') {
>              len--;
> @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
>   */
>  bool qnum_is_equal(const QObject *x, const QObject *y)
>  {
> -    QNum *num_x = qobject_to(QNum, x);
> -    QNum *num_y = qobject_to(QNum, 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:
> diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> index 4105015872..9499b0d845 100644
> --- a/tests/check-qnum.c
> +++ b/tests/check-qnum.c
> @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
>
>      qn = qnum_from_int(value);
>      g_assert(qn != NULL);
> -    g_assert_cmpint(qn->kind, ==, QNUM_I64);
> -    g_assert_cmpint(qn->u.i64, ==, value);
> +    g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> +    g_assert_cmpint(qn->value.u.i64, ==, value);
>      g_assert_cmpint(qn->base.refcnt, ==, 1);
>      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>
> @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
>
>      qn = qnum_from_uint(value);
>      g_assert(qn != NULL);
> -    g_assert_cmpint(qn->kind, ==, QNUM_U64);
> -    g_assert(qn->u.u64 == value);
> +    g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> +    g_assert(qn->value.u.u64 == value);
>      g_assert(qn->base.refcnt == 1);
>      g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
>
> @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
>
>      qn = qnum_from_double(value);
>      g_assert(qn != NULL);
> -    g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> -    g_assert_cmpfloat(qn->u.dbl, ==, value);
> +    g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> +    g_assert_cmpfloat(qn->value.u.dbl, ==, value);
>      g_assert_cmpint(qn->base.refcnt, ==, 1);
>      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>
> @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
>      const int64_t value = 0x1234567890abcdefLL;
>
>      qn = qnum_from_int(value);
> -    g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> +    g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
>
>      qobject_unref(qn);
>  }
> --
> 2.28.0
>
>
>
lgtm otherwise

On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Provide a separate QNumValue type that can be used for QNum value
> > literals without the referencing counting and memory allocation
> > features provided by QObject.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Fix "make check" failure, by updating check-qnum unit test to
> >   use the new struct fields
> > ---
> >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
> >  tests/check-qnum.c      | 14 ++++----
> >  3 files changed, 84 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 55c27b1c24..62fbdfda68 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -46,20 +46,56 @@ typedef enum {
> >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >   * convert under the hood.
> >   */
> > -struct QNum {
> > -    struct QObjectBase_ base;
> > +
> > +/**
> > + * struct QNumValue: the value of a QNum
> > + *
> > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> > + */
> > +typedef struct QNumValue {
> > +    /* private: */
> >      QNumKind kind;
> >      union {
> >          int64_t i64;
> >          uint64_t u64;
> >          double dbl;
> >      } u;
> > +} QNumValue;
> > +
> > +#define QNUM_VAL_INT(value) \
> > +    { .kind = QNUM_I64, .u.i64 = value }
> > +#define QNUM_VAL_UINT(value) \
> > +    { .kind = QNUM_U64, .u.u64 = value }
> > +#define QNUM_VAL_DOUBLE(value) \
> > +    { .kind = QNUM_DOUBLE, .u.dbl = value }
> > +
> > +struct QNum {
> > +    struct QObjectBase_ base;
> > +    QNumValue value;
> >  };
> >
> > +/**
> > + * qnum_from_int(): Create a new QNum from a QNumValue
> >
> 
> Copy-pasta qnum_from_int() -> qnum_from_value()

Oops!  I will fix it in v3, or submit a fixup patch if that's the
only needed change.

> 
> I wonder if there is a check for that kind of mistake, as it may be common.

It looks like kernel-doc ignores what's before the colon in the
summary line.  It probably shouldn't.

> 
> + * @value: QNumValue
> > + *
> > + * Return strong reference.
> > + */
> > +QNum *qnum_from_value(QNumValue value);
> >
> +
> >  QNum *qnum_from_int(int64_t value);
> >  QNum *qnum_from_uint(uint64_t value);
> >  QNum *qnum_from_double(double value);
> >
> > +/**
> > + * qnum_get_value(): Get QNumValue from QNum
> > + * @qn: QNum object
> > + */
> > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> > +{
> > +    return &qn->value;
> > +}
> > +
> >
> 
> Nothing uses that function in this patch. Should be put into use.

It is used in patch 5/8.  Why do you think it's necessary to use
it in internal code too?

> 
>  bool qnum_get_try_int(const QNum *qn, int64_t *val);
> >  int64_t qnum_get_int(const QNum *qn);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > index 69fd9a82d9..f80d4efd76 100644
> > --- a/qobject/qnum.c
> > +++ b/qobject/qnum.c
> > @@ -15,6 +15,15 @@
> >  #include "qemu/osdep.h"
> >  #include "qapi/qmp/qnum.h"
> >
> > +QNum *qnum_from_value(QNumValue value)
> >
> 
> I wonder if it shouldn't be made "inline" in header too (if that can help
> avoid argument copy).

I'm unsure.  I would make it inline (in a separate patch) if
there's evidence it's worth it.  I expect the g_new() call to be
more expensive than any argument copying, though.

> 
> +{
> > +    QNum *qn = g_new(QNum, 1);
> > +
> > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > +    qn->value = value;
> > +    return qn;
> > +}
> > +
> >  /**
> >   * qnum_from_int(): Create a new QNum from an int64_t
> >   * @value: int64_t value
> > @@ -23,13 +32,7 @@
> >   */
> >  QNum *qnum_from_int(int64_t value)
> >  {
> > -    QNum *qn = g_new(QNum, 1);
> > -
> > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > -    qn->kind = QNUM_I64;
> > -    qn->u.i64 = value;
> > -
> > -    return qn;
> > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> >  }
> >
> >  /**
> > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> >   */
> >  QNum *qnum_from_uint(uint64_t value)
> >  {
> > -    QNum *qn = g_new(QNum, 1);
> > -
> > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > -    qn->kind = QNUM_U64;
> > -    qn->u.u64 = value;
> > -
> > -    return qn;
> > +    return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> >  }
> >
> >  /**
> > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> >   */
> >  QNum *qnum_from_double(double value)
> >  {
> > -    QNum *qn = g_new(QNum, 1);
> > -
> > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > -    qn->kind = QNUM_DOUBLE;
> > -    qn->u.dbl = value;
> > -
> > -    return qn;
> > +    return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> >  }
> >
> >  /**
> > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> >   */
> >  bool qnum_get_try_int(const QNum *qn, int64_t *val)
> >  {
> > -    switch (qn->kind) {
> > +    const QNumValue *qv = &qn->value;
> > +
> > +    switch (qv->kind) {
> >      case QNUM_I64:
> > -        *val = qn->u.i64;
> > +        *val = qv->u.i64;
> >          return true;
> >      case QNUM_U64:
> > -        if (qn->u.u64 > INT64_MAX) {
> > +        if (qv->u.u64 > INT64_MAX) {
> >              return false;
> >          }
> > -        *val = qn->u.u64;
> > +        *val = qv->u.u64;
> >          return true;
> >      case QNUM_DOUBLE:
> >          return false;
> > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> >   */
> >  bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> >  {
> > -    switch (qn->kind) {
> > +    const QNumValue *qv = &qn->value;
> > +
> > +    switch (qv->kind) {
> >      case QNUM_I64:
> > -        if (qn->u.i64 < 0) {
> > +        if (qv->u.i64 < 0) {
> >              return false;
> >          }
> > -        *val = qn->u.i64;
> > +        *val = qv->u.i64;
> >          return true;
> >      case QNUM_U64:
> > -        *val = qn->u.u64;
> > +        *val = qv->u.u64;
> >          return true;
> >      case QNUM_DOUBLE:
> >          return false;
> > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> >   */
> >  double qnum_get_double(const QNum *qn)
> >  {
> > -    switch (qn->kind) {
> > +    const QNumValue *qv = &qn->value;
> > +
> > +    switch (qv->kind) {
> >      case QNUM_I64:
> > -        return qn->u.i64;
> > +        return qv->u.i64;
> >      case QNUM_U64:
> > -        return qn->u.u64;
> > +        return qv->u.u64;
> >      case QNUM_DOUBLE:
> > -        return qn->u.dbl;
> > +        return qv->u.dbl;
> >      }
> >
> >      assert(0);
> > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> >
> >  char *qnum_to_string(QNum *qn)
> >  {
> > +    const QNumValue *qv = &qn->value;
> >
> 
> qnum_get_value() ?

I prefer to not hide this behind a function call, in internal
code.  But I don't mind changing it if you think it's important.


> 
>      char *buffer;
> >      int len;
> >
> > -    switch (qn->kind) {
> > +    switch (qv->kind) {
> >      case QNUM_I64:
> > -        return g_strdup_printf("%" PRId64, qn->u.i64);
> > +        return g_strdup_printf("%" PRId64, qv->u.i64);
> >      case QNUM_U64:
> > -        return g_strdup_printf("%" PRIu64, qn->u.u64);
> > +        return g_strdup_printf("%" PRIu64, qv->u.u64);
> >      case QNUM_DOUBLE:
> >          /* FIXME: snprintf() is locale dependent; but JSON requires
> >           * numbers to be formatted as if in the C locale. Dependence
> > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> >           * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> >           * and only rounding to a shorter number if the result would
> >           * still produce the same floating point value.  */
> > -        buffer = g_strdup_printf("%f" , qn->u.dbl);
> > +        buffer = g_strdup_printf("%f" , qv->u.dbl);
> >          len = strlen(buffer);
> >          while (len > 0 && buffer[len - 1] == '0') {
> >              len--;
> > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> >   */
> >  bool qnum_is_equal(const QObject *x, const QObject *y)
> >  {
> > -    QNum *num_x = qobject_to(QNum, x);
> > -    QNum *num_y = qobject_to(QNum, 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:
> > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> > index 4105015872..9499b0d845 100644
> > --- a/tests/check-qnum.c
> > +++ b/tests/check-qnum.c
> > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> >
> >      qn = qnum_from_int(value);
> >      g_assert(qn != NULL);
> > -    g_assert_cmpint(qn->kind, ==, QNUM_I64);
> > -    g_assert_cmpint(qn->u.i64, ==, value);
> > +    g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> > +    g_assert_cmpint(qn->value.u.i64, ==, value);
> >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >
> > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> >
> >      qn = qnum_from_uint(value);
> >      g_assert(qn != NULL);
> > -    g_assert_cmpint(qn->kind, ==, QNUM_U64);
> > -    g_assert(qn->u.u64 == value);
> > +    g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> > +    g_assert(qn->value.u.u64 == value);
> >      g_assert(qn->base.refcnt == 1);
> >      g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> >
> > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> >
> >      qn = qnum_from_double(value);
> >      g_assert(qn != NULL);
> > -    g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> > -    g_assert_cmpfloat(qn->u.dbl, ==, value);
> > +    g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> > +    g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >
> > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> >      const int64_t value = 0x1234567890abcdefLL;
> >
> >      qn = qnum_from_int(value);
> > -    g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> > +    g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> >
> >      qobject_unref(qn);
> >  }
> > --
> > 2.28.0
> >
> >
> >
> lgtm otherwise

Thanks!

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

> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> >
> > > Provide a separate QNumValue type that can be used for QNum value
> > > literals without the referencing counting and memory allocation
> > > features provided by QObject.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes v1 -> v2:
> > > * Fix "make check" failure, by updating check-qnum unit test to
> > >   use the new struct fields
> > > ---
> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> > >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
> > >  tests/check-qnum.c      | 14 ++++----
> > >  3 files changed, 84 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > > index 55c27b1c24..62fbdfda68 100644
> > > --- a/include/qapi/qmp/qnum.h
> > > +++ b/include/qapi/qmp/qnum.h
> > > @@ -46,20 +46,56 @@ typedef enum {
> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> > >   * convert under the hood.
> > >   */
> > > -struct QNum {
> > > -    struct QObjectBase_ base;
> > > +
> > > +/**
> > > + * struct QNumValue: the value of a QNum
> > > + *
> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> > > + */
> > > +typedef struct QNumValue {
> > > +    /* private: */
> > >      QNumKind kind;
> > >      union {
> > >          int64_t i64;
> > >          uint64_t u64;
> > >          double dbl;
> > >      } u;
> > > +} QNumValue;
> > > +
> > > +#define QNUM_VAL_INT(value) \
> > > +    { .kind = QNUM_I64, .u.i64 = value }
> > > +#define QNUM_VAL_UINT(value) \
> > > +    { .kind = QNUM_U64, .u.u64 = value }
> > > +#define QNUM_VAL_DOUBLE(value) \
> > > +    { .kind = QNUM_DOUBLE, .u.dbl = value }
> > > +
> > > +struct QNum {
> > > +    struct QObjectBase_ base;
> > > +    QNumValue value;
> > >  };
> > >
> > > +/**
> > > + * qnum_from_int(): Create a new QNum from a QNumValue
> > >
> >
> > Copy-pasta qnum_from_int() -> qnum_from_value()
>
> Oops!  I will fix it in v3, or submit a fixup patch if that's the
> only needed change.
>
> >
> > I wonder if there is a check for that kind of mistake, as it may be
> common.
>
> It looks like kernel-doc ignores what's before the colon in the
> summary line.  It probably shouldn't.
>
> >
> > + * @value: QNumValue
> > > + *
> > > + * Return strong reference.
> > > + */
> > > +QNum *qnum_from_value(QNumValue value);
> > >
> > +
> > >  QNum *qnum_from_int(int64_t value);
> > >  QNum *qnum_from_uint(uint64_t value);
> > >  QNum *qnum_from_double(double value);
> > >
> > > +/**
> > > + * qnum_get_value(): Get QNumValue from QNum
> > > + * @qn: QNum object
> > > + */
> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> > > +{
> > > +    return &qn->value;
> > > +}
> > > +
> > >
> >
> > Nothing uses that function in this patch. Should be put into use.
>
> It is used in patch 5/8.  Why do you think it's necessary to use
> it in internal code too?
>

Not necessarily, just want to make sure we don't introduce dead code. Fine
it's used later, perhaps worth noting in the commit message.

>
> >
> >  bool qnum_get_try_int(const QNum *qn, int64_t *val);
> > >  int64_t qnum_get_int(const QNum *qn);
> > >
> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > > index 69fd9a82d9..f80d4efd76 100644
> > > --- a/qobject/qnum.c
> > > +++ b/qobject/qnum.c
> > > @@ -15,6 +15,15 @@
> > >  #include "qemu/osdep.h"
> > >  #include "qapi/qmp/qnum.h"
> > >
> > > +QNum *qnum_from_value(QNumValue value)
> > >
> >
> > I wonder if it shouldn't be made "inline" in header too (if that can help
> > avoid argument copy).
>
> I'm unsure.  I would make it inline (in a separate patch) if
> there's evidence it's worth it.  I expect the g_new() call to be
> more expensive than any argument copying, though.
>

ok


> >
> > +{
> > > +    QNum *qn = g_new(QNum, 1);
> > > +
> > > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > +    qn->value = value;
> > > +    return qn;
> > > +}
> > > +
> > >  /**
> > >   * qnum_from_int(): Create a new QNum from an int64_t
> > >   * @value: int64_t value
> > > @@ -23,13 +32,7 @@
> > >   */
> > >  QNum *qnum_from_int(int64_t value)
> > >  {
> > > -    QNum *qn = g_new(QNum, 1);
> > > -
> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > -    qn->kind = QNUM_I64;
> > > -    qn->u.i64 = value;
> > > -
> > > -    return qn;
> > > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> > >  }
> > >
> > >  /**
> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> > >   */
> > >  QNum *qnum_from_uint(uint64_t value)
> > >  {
> > > -    QNum *qn = g_new(QNum, 1);
> > > -
> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > -    qn->kind = QNUM_U64;
> > > -    qn->u.u64 = value;
> > > -
> > > -    return qn;
> > > +    return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> > >  }
> > >
> > >  /**
> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> > >   */
> > >  QNum *qnum_from_double(double value)
> > >  {
> > > -    QNum *qn = g_new(QNum, 1);
> > > -
> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > -    qn->kind = QNUM_DOUBLE;
> > > -    qn->u.dbl = value;
> > > -
> > > -    return qn;
> > > +    return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> > >  }
> > >
> > >  /**
> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> > >   */
> > >  bool qnum_get_try_int(const QNum *qn, int64_t *val)
> > >  {
> > > -    switch (qn->kind) {
> > > +    const QNumValue *qv = &qn->value;
> > > +
> > > +    switch (qv->kind) {
> > >      case QNUM_I64:
> > > -        *val = qn->u.i64;
> > > +        *val = qv->u.i64;
> > >          return true;
> > >      case QNUM_U64:
> > > -        if (qn->u.u64 > INT64_MAX) {
> > > +        if (qv->u.u64 > INT64_MAX) {
> > >              return false;
> > >          }
> > > -        *val = qn->u.u64;
> > > +        *val = qv->u.u64;
> > >          return true;
> > >      case QNUM_DOUBLE:
> > >          return false;
> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> > >   */
> > >  bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> > >  {
> > > -    switch (qn->kind) {
> > > +    const QNumValue *qv = &qn->value;
> > > +
> > > +    switch (qv->kind) {
> > >      case QNUM_I64:
> > > -        if (qn->u.i64 < 0) {
> > > +        if (qv->u.i64 < 0) {
> > >              return false;
> > >          }
> > > -        *val = qn->u.i64;
> > > +        *val = qv->u.i64;
> > >          return true;
> > >      case QNUM_U64:
> > > -        *val = qn->u.u64;
> > > +        *val = qv->u.u64;
> > >          return true;
> > >      case QNUM_DOUBLE:
> > >          return false;
> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> > >   */
> > >  double qnum_get_double(const QNum *qn)
> > >  {
> > > -    switch (qn->kind) {
> > > +    const QNumValue *qv = &qn->value;
> > > +
> > > +    switch (qv->kind) {
> > >      case QNUM_I64:
> > > -        return qn->u.i64;
> > > +        return qv->u.i64;
> > >      case QNUM_U64:
> > > -        return qn->u.u64;
> > > +        return qv->u.u64;
> > >      case QNUM_DOUBLE:
> > > -        return qn->u.dbl;
> > > +        return qv->u.dbl;
> > >      }
> > >
> > >      assert(0);
> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> > >
> > >  char *qnum_to_string(QNum *qn)
> > >  {
> > > +    const QNumValue *qv = &qn->value;
> > >
> >
> > qnum_get_value() ?
>
> I prefer to not hide this behind a function call, in internal
> code.  But I don't mind changing it if you think it's important.
>

no, it's ok to me


>
> >
> >      char *buffer;
> > >      int len;
> > >
> > > -    switch (qn->kind) {
> > > +    switch (qv->kind) {
> > >      case QNUM_I64:
> > > -        return g_strdup_printf("%" PRId64, qn->u.i64);
> > > +        return g_strdup_printf("%" PRId64, qv->u.i64);
> > >      case QNUM_U64:
> > > -        return g_strdup_printf("%" PRIu64, qn->u.u64);
> > > +        return g_strdup_printf("%" PRIu64, qv->u.u64);
> > >      case QNUM_DOUBLE:
> > >          /* FIXME: snprintf() is locale dependent; but JSON requires
> > >           * numbers to be formatted as if in the C locale. Dependence
> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> > >           * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> > >           * and only rounding to a shorter number if the result would
> > >           * still produce the same floating point value.  */
> > > -        buffer = g_strdup_printf("%f" , qn->u.dbl);
> > > +        buffer = g_strdup_printf("%f" , qv->u.dbl);
> > >          len = strlen(buffer);
> > >          while (len > 0 && buffer[len - 1] == '0') {
> > >              len--;
> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> > >   */
> > >  bool qnum_is_equal(const QObject *x, const QObject *y)
> > >  {
> > > -    QNum *num_x = qobject_to(QNum, x);
> > > -    QNum *num_y = qobject_to(QNum, 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:
> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> > > index 4105015872..9499b0d845 100644
> > > --- a/tests/check-qnum.c
> > > +++ b/tests/check-qnum.c
> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> > >
> > >      qn = qnum_from_int(value);
> > >      g_assert(qn != NULL);
> > > -    g_assert_cmpint(qn->kind, ==, QNUM_I64);
> > > -    g_assert_cmpint(qn->u.i64, ==, value);
> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> > > +    g_assert_cmpint(qn->value.u.i64, ==, value);
> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> > >
> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> > >
> > >      qn = qnum_from_uint(value);
> > >      g_assert(qn != NULL);
> > > -    g_assert_cmpint(qn->kind, ==, QNUM_U64);
> > > -    g_assert(qn->u.u64 == value);
> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> > > +    g_assert(qn->value.u.u64 == value);
> > >      g_assert(qn->base.refcnt == 1);
> > >      g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> > >
> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> > >
> > >      qn = qnum_from_double(value);
> > >      g_assert(qn != NULL);
> > > -    g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> > > -    g_assert_cmpfloat(qn->u.dbl, ==, value);
> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> > > +    g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> > >
> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> > >      const int64_t value = 0x1234567890abcdefLL;
> > >
> > >      qn = qnum_from_int(value);
> > > -    g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> > > +    g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> > >
> > >      qobject_unref(qn);
> > >  }
> > > --
> > > 2.28.0
> > >
> > >
> > >
> > lgtm otherwise
>
> Thanks!
>
> --
> Eduardo
>
>

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
>> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
>> wrote:
>> >
>> > > Provide a separate QNumValue type that can be used for QNum value
>> > > literals without the referencing counting and memory allocation
>> > > features provided by QObject.
>> > >
>> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > > ---
>> > > Changes v1 -> v2:
>> > > * Fix "make check" failure, by updating check-qnum unit test to
>> > >   use the new struct fields
>> > > ---
>> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
>> > >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
>> > >  tests/check-qnum.c      | 14 ++++----
>> > >  3 files changed, 84 insertions(+), 48 deletions(-)
>> > >
>> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
>> > > index 55c27b1c24..62fbdfda68 100644
>> > > --- a/include/qapi/qmp/qnum.h
>> > > +++ b/include/qapi/qmp/qnum.h
>> > > @@ -46,20 +46,56 @@ typedef enum {
>> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>> > >   * convert under the hood.
>> > >   */
>> > > -struct QNum {
>> > > -    struct QObjectBase_ base;
>> > > +
>> > > +/**
>> > > + * struct QNumValue: the value of a QNum
>> > > + *
>> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
>> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
>> > > + */
>> > > +typedef struct QNumValue {
>> > > +    /* private: */

Do we care?

>> > >      QNumKind kind;
>> > >      union {
>> > >          int64_t i64;
>> > >          uint64_t u64;
>> > >          double dbl;
>> > >      } u;
>> > > +} QNumValue;
>> > > +
>> > > +#define QNUM_VAL_INT(value) \
>> > > +    { .kind = QNUM_I64, .u.i64 = value }
>> > > +#define QNUM_VAL_UINT(value) \
>> > > +    { .kind = QNUM_U64, .u.u64 = value }
>> > > +#define QNUM_VAL_DOUBLE(value) \
>> > > +    { .kind = QNUM_DOUBLE, .u.dbl = value }
>> > > +
>> > > +struct QNum {
>> > > +    struct QObjectBase_ base;
>> > > +    QNumValue value;
>> > >  };
>> > >
>> > > +/**
>> > > + * qnum_from_int(): Create a new QNum from a QNumValue
>> > >
>> >
>> > Copy-pasta qnum_from_int() -> qnum_from_value()
>>
>> Oops!  I will fix it in v3, or submit a fixup patch if that's the
>> only needed change.
>>
>> >
>> > I wonder if there is a check for that kind of mistake, as it may be
>> common.
>>
>> It looks like kernel-doc ignores what's before the colon in the
>> summary line.  It probably shouldn't.
>>
>> >
>> > + * @value: QNumValue
>> > > + *
>> > > + * Return strong reference.
>> > > + */
>> > > +QNum *qnum_from_value(QNumValue value);
>> > >
>> > +
>> > >  QNum *qnum_from_int(int64_t value);
>> > >  QNum *qnum_from_uint(uint64_t value);
>> > >  QNum *qnum_from_double(double value);
>> > >
>> > > +/**
>> > > + * qnum_get_value(): Get QNumValue from QNum
>> > > + * @qn: QNum object
>> > > + */
>> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
>> > > +{
>> > > +    return &qn->value;
>> > > +}
>> > > +
>> > >
>> >
>> > Nothing uses that function in this patch. Should be put into use.
>>
>> It is used in patch 5/8.  Why do you think it's necessary to use
>> it in internal code too?
>>
>
> Not necessarily, just want to make sure we don't introduce dead code. Fine
> it's used later, perhaps worth noting in the commit message.

Yes, use later in the same series suffices.

The whole patch makes sense only because later patches put QNumValue to
use.  The commit message could perhaps be more explicit about that.  No
need to be explicit about every new function, though.

That said, I wouldn't bother with a getter function.  Yes, we have
similar trivial getters for other QFoo.  I don't care for them, either.
The actual structure of these data types is trivial, and not worth
hiding.

Also, I'm wary of inline functions in headers.  *Especially* when they
require additional #include (which this one doesn't).  I accept them
when they provide a worthwhile performance improvement.  Guesses don't
count as evidence :)

>>
>> >
>> >  bool qnum_get_try_int(const QNum *qn, int64_t *val);
>> > >  int64_t qnum_get_int(const QNum *qn);
>> > >
>> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
>> > > index 69fd9a82d9..f80d4efd76 100644
>> > > --- a/qobject/qnum.c
>> > > +++ b/qobject/qnum.c
>> > > @@ -15,6 +15,15 @@
>> > >  #include "qemu/osdep.h"
>> > >  #include "qapi/qmp/qnum.h"
>> > >
>> > > +QNum *qnum_from_value(QNumValue value)
>> > >
>> >
>> > I wonder if it shouldn't be made "inline" in header too (if that can help
>> > avoid argument copy).
>>
>> I'm unsure.  I would make it inline (in a separate patch) if
>> there's evidence it's worth it.  I expect the g_new() call to be
>> more expensive than any argument copying, though.
>
> ok

I'm with Eduardo here.

>> >
>> > +{
>> > > +    QNum *qn = g_new(QNum, 1);
>> > > +
>> > > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > +    qn->value = value;
>> > > +    return qn;
>> > > +}
>> > > +
>> > >  /**
>> > >   * qnum_from_int(): Create a new QNum from an int64_t
>> > >   * @value: int64_t value
>> > > @@ -23,13 +32,7 @@
>> > >   */
>> > >  QNum *qnum_from_int(int64_t value)
>> > >  {
>> > > -    QNum *qn = g_new(QNum, 1);
>> > > -
>> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > -    qn->kind = QNUM_I64;
>> > > -    qn->u.i64 = value;
>> > > -
>> > > -    return qn;
>> > > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));

No space between between (type) and its operand, please.

Could we lift the cast into the macro somehow?

>> > >  }
>> > >
>> > >  /**
>> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
>> > >   */
>> > >  QNum *qnum_from_uint(uint64_t value)
>> > >  {
>> > > -    QNum *qn = g_new(QNum, 1);
>> > > -
>> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > -    qn->kind = QNUM_U64;
>> > > -    qn->u.u64 = value;
>> > > -
>> > > -    return qn;
>> > > +    return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
>> > >  }
>> > >
>> > >  /**
>> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
>> > >   */
>> > >  QNum *qnum_from_double(double value)
>> > >  {
>> > > -    QNum *qn = g_new(QNum, 1);
>> > > -
>> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > -    qn->kind = QNUM_DOUBLE;
>> > > -    qn->u.dbl = value;
>> > > -
>> > > -    return qn;
>> > > +    return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
>> > >  }
>> > >
>> > >  /**
>> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
>> > >   */
>> > >  bool qnum_get_try_int(const QNum *qn, int64_t *val)
>> > >  {
>> > > -    switch (qn->kind) {
>> > > +    const QNumValue *qv = &qn->value;
>> > > +
>> > > +    switch (qv->kind) {
>> > >      case QNUM_I64:
>> > > -        *val = qn->u.i64;
>> > > +        *val = qv->u.i64;
>> > >          return true;
>> > >      case QNUM_U64:
>> > > -        if (qn->u.u64 > INT64_MAX) {
>> > > +        if (qv->u.u64 > INT64_MAX) {
>> > >              return false;
>> > >          }
>> > > -        *val = qn->u.u64;
>> > > +        *val = qv->u.u64;
>> > >          return true;
>> > >      case QNUM_DOUBLE:
>> > >          return false;

Here you add a new variable to shorten "qn->value." to "qv->".  In
tests/check-qnum.c you don't.  I'm not sure the variable is worthwhile.

>> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
>> > >   */
>> > >  bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
>> > >  {
>> > > -    switch (qn->kind) {
>> > > +    const QNumValue *qv = &qn->value;
>> > > +
>> > > +    switch (qv->kind) {
>> > >      case QNUM_I64:
>> > > -        if (qn->u.i64 < 0) {
>> > > +        if (qv->u.i64 < 0) {
>> > >              return false;
>> > >          }
>> > > -        *val = qn->u.i64;
>> > > +        *val = qv->u.i64;
>> > >          return true;
>> > >      case QNUM_U64:
>> > > -        *val = qn->u.u64;
>> > > +        *val = qv->u.u64;
>> > >          return true;
>> > >      case QNUM_DOUBLE:
>> > >          return false;
>> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
>> > >   */
>> > >  double qnum_get_double(const QNum *qn)
>> > >  {
>> > > -    switch (qn->kind) {
>> > > +    const QNumValue *qv = &qn->value;
>> > > +
>> > > +    switch (qv->kind) {
>> > >      case QNUM_I64:
>> > > -        return qn->u.i64;
>> > > +        return qv->u.i64;
>> > >      case QNUM_U64:
>> > > -        return qn->u.u64;
>> > > +        return qv->u.u64;
>> > >      case QNUM_DOUBLE:
>> > > -        return qn->u.dbl;
>> > > +        return qv->u.dbl;
>> > >      }
>> > >
>> > >      assert(0);
>> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
>> > >
>> > >  char *qnum_to_string(QNum *qn)
>> > >  {
>> > > +    const QNumValue *qv = &qn->value;
>> > >
>> >
>> > qnum_get_value() ?
>>
>> I prefer to not hide this behind a function call, in internal
>> code.  But I don't mind changing it if you think it's important.

Me too.  Even in external code.

> no, it's ok to me
>
>
>>
>> >
>> >      char *buffer;
>> > >      int len;
>> > >
>> > > -    switch (qn->kind) {
>> > > +    switch (qv->kind) {
>> > >      case QNUM_I64:
>> > > -        return g_strdup_printf("%" PRId64, qn->u.i64);
>> > > +        return g_strdup_printf("%" PRId64, qv->u.i64);
>> > >      case QNUM_U64:
>> > > -        return g_strdup_printf("%" PRIu64, qn->u.u64);
>> > > +        return g_strdup_printf("%" PRIu64, qv->u.u64);
>> > >      case QNUM_DOUBLE:
>> > >          /* FIXME: snprintf() is locale dependent; but JSON requires
>> > >           * numbers to be formatted as if in the C locale. Dependence
>> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
>> > >           * rounding errors; we should be using DBL_DECIMAL_DIG (17),
>> > >           * and only rounding to a shorter number if the result would
>> > >           * still produce the same floating point value.  */
>> > > -        buffer = g_strdup_printf("%f" , qn->u.dbl);
>> > > +        buffer = g_strdup_printf("%f" , qv->u.dbl);
>> > >          len = strlen(buffer);
>> > >          while (len > 0 && buffer[len - 1] == '0') {
>> > >              len--;
>> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
>> > >   */
>> > >  bool qnum_is_equal(const QObject *x, const QObject *y)
>> > >  {
>> > > -    QNum *num_x = qobject_to(QNum, x);
>> > > -    QNum *num_y = qobject_to(QNum, y);
>> > > +    const QNum *qnum_x = qobject_to(QNum, x);
>> > > +    const QNum *qnum_y = qobject_to(QNum, y);

A bit of drive-by constification.  Okay.

>> > > +    const QNumValue *num_x = &qnum_x->value;
>> > > +    const QNumValue *num_y = &qnum_y->value;
>> > >
>> > >      switch (num_x->kind) {
>> > >      case QNUM_I64:
>> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
>> > > index 4105015872..9499b0d845 100644
>> > > --- a/tests/check-qnum.c
>> > > +++ b/tests/check-qnum.c
>> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
>> > >
>> > >      qn = qnum_from_int(value);
>> > >      g_assert(qn != NULL);
>> > > -    g_assert_cmpint(qn->kind, ==, QNUM_I64);
>> > > -    g_assert_cmpint(qn->u.i64, ==, value);
>> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
>> > > +    g_assert_cmpint(qn->value.u.i64, ==, value);
>> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
>> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>> > >
>> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
>> > >
>> > >      qn = qnum_from_uint(value);
>> > >      g_assert(qn != NULL);
>> > > -    g_assert_cmpint(qn->kind, ==, QNUM_U64);
>> > > -    g_assert(qn->u.u64 == value);
>> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
>> > > +    g_assert(qn->value.u.u64 == value);
>> > >      g_assert(qn->base.refcnt == 1);
>> > >      g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
>> > >
>> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
>> > >
>> > >      qn = qnum_from_double(value);
>> > >      g_assert(qn != NULL);
>> > > -    g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
>> > > -    g_assert_cmpfloat(qn->u.dbl, ==, value);
>> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
>> > > +    g_assert_cmpfloat(qn->value.u.dbl, ==, value);
>> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
>> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>> > >
>> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
>> > >      const int64_t value = 0x1234567890abcdefLL;
>> > >
>> > >      qn = qnum_from_int(value);
>> > > -    g_assert_cmpint((int64_t) qn->u.i64, ==, value);
>> > > +    g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
>> > >
>> > >      qobject_unref(qn);
>> > >  }
>> > > --
>> > > 2.28.0
>> > >
>> > >
>> > >
>> > lgtm otherwise
>>
>> Thanks!
>>
>> --
>> Eduardo
>>
>>

On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> >> wrote:
> >> >
> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> > > literals without the referencing counting and memory allocation
> >> > > features provided by QObject.
> >> > >
> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > > ---
> >> > > Changes v1 -> v2:
> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> > >   use the new struct fields
> >> > > ---
> >> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >> > >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
> >> > >  tests/check-qnum.c      | 14 ++++----
> >> > >  3 files changed, 84 insertions(+), 48 deletions(-)
> >> > >
> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> > > index 55c27b1c24..62fbdfda68 100644
> >> > > --- a/include/qapi/qmp/qnum.h
> >> > > +++ b/include/qapi/qmp/qnum.h
> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >> > >   * convert under the hood.
> >> > >   */
> >> > > -struct QNum {
> >> > > -    struct QObjectBase_ base;
> >> > > +
> >> > > +/**
> >> > > + * struct QNumValue: the value of a QNum
> >> > > + *
> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> > > + */
> >> > > +typedef struct QNumValue {
> >> > > +    /* private: */
> 
> Do we care?

Are you asking if we want to make them private, or if we want to
document them as private (assuming they are).

The answer to the latter is yes ("private:" is an indication to
kernel-doc).  The answer to the former seems to be "no", based on
your other comments.

Or maybe `kind` should be public and `u` should be private?

> 
> >> > >      QNumKind kind;
> >> > >      union {
> >> > >          int64_t i64;
> >> > >          uint64_t u64;
> >> > >          double dbl;
> >> > >      } u;
> >> > > +} QNumValue;
> >> > > +
> >> > > +#define QNUM_VAL_INT(value) \
> >> > > +    { .kind = QNUM_I64, .u.i64 = value }
> >> > > +#define QNUM_VAL_UINT(value) \
> >> > > +    { .kind = QNUM_U64, .u.u64 = value }
> >> > > +#define QNUM_VAL_DOUBLE(value) \
> >> > > +    { .kind = QNUM_DOUBLE, .u.dbl = value }
> >> > > +
> >> > > +struct QNum {
> >> > > +    struct QObjectBase_ base;
> >> > > +    QNumValue value;
> >> > >  };
> >> > >
> >> > > +/**
> >> > > + * qnum_from_int(): Create a new QNum from a QNumValue
> >> > >
> >> >
> >> > Copy-pasta qnum_from_int() -> qnum_from_value()
> >>
> >> Oops!  I will fix it in v3, or submit a fixup patch if that's the
> >> only needed change.
> >>
> >> >
> >> > I wonder if there is a check for that kind of mistake, as it may be
> >> common.
> >>
> >> It looks like kernel-doc ignores what's before the colon in the
> >> summary line.  It probably shouldn't.
> >>
> >> >
> >> > + * @value: QNumValue
> >> > > + *
> >> > > + * Return strong reference.
> >> > > + */
> >> > > +QNum *qnum_from_value(QNumValue value);
> >> > >
> >> > +
> >> > >  QNum *qnum_from_int(int64_t value);
> >> > >  QNum *qnum_from_uint(uint64_t value);
> >> > >  QNum *qnum_from_double(double value);
> >> > >
> >> > > +/**
> >> > > + * qnum_get_value(): Get QNumValue from QNum
> >> > > + * @qn: QNum object
> >> > > + */
> >> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> >> > > +{
> >> > > +    return &qn->value;
> >> > > +}
> >> > > +
> >> > >
> >> >
> >> > Nothing uses that function in this patch. Should be put into use.
> >>
> >> It is used in patch 5/8.  Why do you think it's necessary to use
> >> it in internal code too?
> >>
> >
> > Not necessarily, just want to make sure we don't introduce dead code. Fine
> > it's used later, perhaps worth noting in the commit message.
> 
> Yes, use later in the same series suffices.
> 
> The whole patch makes sense only because later patches put QNumValue to
> use.  The commit message could perhaps be more explicit about that.  No
> need to be explicit about every new function, though.
> 
> That said, I wouldn't bother with a getter function.  Yes, we have
> similar trivial getters for other QFoo.  I don't care for them, either.
> The actual structure of these data types is trivial, and not worth
> hiding.
> 
> Also, I'm wary of inline functions in headers.  *Especially* when they
> require additional #include (which this one doesn't).  I accept them
> when they provide a worthwhile performance improvement.  Guesses don't
> count as evidence :)
> 
> >>
> >> >
> >> >  bool qnum_get_try_int(const QNum *qn, int64_t *val);
> >> > >  int64_t qnum_get_int(const QNum *qn);
> >> > >
> >> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> >> > > index 69fd9a82d9..f80d4efd76 100644
> >> > > --- a/qobject/qnum.c
> >> > > +++ b/qobject/qnum.c
> >> > > @@ -15,6 +15,15 @@
> >> > >  #include "qemu/osdep.h"
> >> > >  #include "qapi/qmp/qnum.h"
> >> > >
> >> > > +QNum *qnum_from_value(QNumValue value)
> >> > >
> >> >
> >> > I wonder if it shouldn't be made "inline" in header too (if that can help
> >> > avoid argument copy).
> >>
> >> I'm unsure.  I would make it inline (in a separate patch) if
> >> there's evidence it's worth it.  I expect the g_new() call to be
> >> more expensive than any argument copying, though.
> >
> > ok
> 
> I'm with Eduardo here.
> 
> >> >
> >> > +{
> >> > > +    QNum *qn = g_new(QNum, 1);
> >> > > +
> >> > > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > +    qn->value = value;
> >> > > +    return qn;
> >> > > +}
> >> > > +
> >> > >  /**
> >> > >   * qnum_from_int(): Create a new QNum from an int64_t
> >> > >   * @value: int64_t value
> >> > > @@ -23,13 +32,7 @@
> >> > >   */
> >> > >  QNum *qnum_from_int(int64_t value)
> >> > >  {
> >> > > -    QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > -    qn->kind = QNUM_I64;
> >> > > -    qn->u.i64 = value;
> >> > > -
> >> > > -    return qn;
> >> > > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> 
> No space between between (type) and its operand, please.
> 
> Could we lift the cast into the macro somehow?

I think we can.  I had thought the cast in the macro would break
usage as static variable initializers.  I was wrong.

> 
> >> > >  }
> >> > >
> >> > >  /**
> >> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> >> > >   */
> >> > >  QNum *qnum_from_uint(uint64_t value)
> >> > >  {
> >> > > -    QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > -    qn->kind = QNUM_U64;
> >> > > -    qn->u.u64 = value;
> >> > > -
> >> > > -    return qn;
> >> > > +    return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> >> > >  }
> >> > >
> >> > >  /**
> >> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> >> > >   */
> >> > >  QNum *qnum_from_double(double value)
> >> > >  {
> >> > > -    QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > -    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > -    qn->kind = QNUM_DOUBLE;
> >> > > -    qn->u.dbl = value;
> >> > > -
> >> > > -    return qn;
> >> > > +    return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> >> > >  }
> >> > >
> >> > >  /**
> >> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> >> > >   */
> >> > >  bool qnum_get_try_int(const QNum *qn, int64_t *val)
> >> > >  {
> >> > > -    switch (qn->kind) {
> >> > > +    const QNumValue *qv = &qn->value;
> >> > > +
> >> > > +    switch (qv->kind) {
> >> > >      case QNUM_I64:
> >> > > -        *val = qn->u.i64;
> >> > > +        *val = qv->u.i64;
> >> > >          return true;
> >> > >      case QNUM_U64:
> >> > > -        if (qn->u.u64 > INT64_MAX) {
> >> > > +        if (qv->u.u64 > INT64_MAX) {
> >> > >              return false;
> >> > >          }
> >> > > -        *val = qn->u.u64;
> >> > > +        *val = qv->u.u64;
> >> > >          return true;
> >> > >      case QNUM_DOUBLE:
> >> > >          return false;
> 
> Here you add a new variable to shorten "qn->value." to "qv->".  In
> tests/check-qnum.c you don't.  I'm not sure the variable is worthwhile.

The variable is worthwhile if it becomes a parameter to
qnum_value_get_try_int().

If we don't create a qnum_value_get_try_int() function, I still
think the variable makes the code easier to read, but I'm not too
attached to it.

> 
> >> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> >> > >   */
> >> > >  bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> >> > >  {
> >> > > -    switch (qn->kind) {
> >> > > +    const QNumValue *qv = &qn->value;
> >> > > +
> >> > > +    switch (qv->kind) {
> >> > >      case QNUM_I64:
> >> > > -        if (qn->u.i64 < 0) {
> >> > > +        if (qv->u.i64 < 0) {
> >> > >              return false;
> >> > >          }
> >> > > -        *val = qn->u.i64;
> >> > > +        *val = qv->u.i64;
> >> > >          return true;
> >> > >      case QNUM_U64:
> >> > > -        *val = qn->u.u64;
> >> > > +        *val = qv->u.u64;
> >> > >          return true;
> >> > >      case QNUM_DOUBLE:
> >> > >          return false;
> >> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> >> > >   */
> >> > >  double qnum_get_double(const QNum *qn)
> >> > >  {
> >> > > -    switch (qn->kind) {
> >> > > +    const QNumValue *qv = &qn->value;
> >> > > +
> >> > > +    switch (qv->kind) {
> >> > >      case QNUM_I64:
> >> > > -        return qn->u.i64;
> >> > > +        return qv->u.i64;
> >> > >      case QNUM_U64:
> >> > > -        return qn->u.u64;
> >> > > +        return qv->u.u64;
> >> > >      case QNUM_DOUBLE:
> >> > > -        return qn->u.dbl;
> >> > > +        return qv->u.dbl;
> >> > >      }
> >> > >
> >> > >      assert(0);
> >> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> >> > >
> >> > >  char *qnum_to_string(QNum *qn)
> >> > >  {
> >> > > +    const QNumValue *qv = &qn->value;
> >> > >
> >> >
> >> > qnum_get_value() ?
> >>
> >> I prefer to not hide this behind a function call, in internal
> >> code.  But I don't mind changing it if you think it's important.
> 
> Me too.  Even in external code.

Understood.

> 
> > no, it's ok to me
> >
> >
> >>
> >> >
> >> >      char *buffer;
> >> > >      int len;
> >> > >
> >> > > -    switch (qn->kind) {
> >> > > +    switch (qv->kind) {
> >> > >      case QNUM_I64:
> >> > > -        return g_strdup_printf("%" PRId64, qn->u.i64);
> >> > > +        return g_strdup_printf("%" PRId64, qv->u.i64);
> >> > >      case QNUM_U64:
> >> > > -        return g_strdup_printf("%" PRIu64, qn->u.u64);
> >> > > +        return g_strdup_printf("%" PRIu64, qv->u.u64);
> >> > >      case QNUM_DOUBLE:
> >> > >          /* FIXME: snprintf() is locale dependent; but JSON requires
> >> > >           * numbers to be formatted as if in the C locale. Dependence
> >> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> >> > >           * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> >> > >           * and only rounding to a shorter number if the result would
> >> > >           * still produce the same floating point value.  */
> >> > > -        buffer = g_strdup_printf("%f" , qn->u.dbl);
> >> > > +        buffer = g_strdup_printf("%f" , qv->u.dbl);
> >> > >          len = strlen(buffer);
> >> > >          while (len > 0 && buffer[len - 1] == '0') {
> >> > >              len--;
> >> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> >> > >   */
> >> > >  bool qnum_is_equal(const QObject *x, const QObject *y)
> >> > >  {
> >> > > -    QNum *num_x = qobject_to(QNum, x);
> >> > > -    QNum *num_y = qobject_to(QNum, y);
> >> > > +    const QNum *qnum_x = qobject_to(QNum, x);
> >> > > +    const QNum *qnum_y = qobject_to(QNum, y);
> 
> A bit of drive-by constification.  Okay.

In my defense, I will argue that those are four new variables.
the variables `QNum *num_x, *num_y` don't exist anymore.

Anyway, I will probably keep the `QNumValue *num_x, *num_y`
variables untouched in the next version, and choose another name
for the new QNumValue variables (which will become
qnum_value_is_equal() parameters in the next patch).

> 
> >> > > +    const QNumValue *num_x = &qnum_x->value;
> >> > > +    const QNumValue *num_y = &qnum_y->value;
> >> > >
> >> > >      switch (num_x->kind) {
> >> > >      case QNUM_I64:
> >> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> >> > > index 4105015872..9499b0d845 100644
> >> > > --- a/tests/check-qnum.c
> >> > > +++ b/tests/check-qnum.c
> >> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> >> > >
> >> > >      qn = qnum_from_int(value);
> >> > >      g_assert(qn != NULL);
> >> > > -    g_assert_cmpint(qn->kind, ==, QNUM_I64);
> >> > > -    g_assert_cmpint(qn->u.i64, ==, value);
> >> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> >> > > +    g_assert_cmpint(qn->value.u.i64, ==, value);
> >> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> >> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >> > >
> >> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> >> > >
> >> > >      qn = qnum_from_uint(value);
> >> > >      g_assert(qn != NULL);
> >> > > -    g_assert_cmpint(qn->kind, ==, QNUM_U64);
> >> > > -    g_assert(qn->u.u64 == value);
> >> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> >> > > +    g_assert(qn->value.u.u64 == value);
> >> > >      g_assert(qn->base.refcnt == 1);
> >> > >      g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> >> > >
> >> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> >> > >
> >> > >      qn = qnum_from_double(value);
> >> > >      g_assert(qn != NULL);
> >> > > -    g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> >> > > -    g_assert_cmpfloat(qn->u.dbl, ==, value);
> >> > > +    g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> >> > > +    g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> >> > >      g_assert_cmpint(qn->base.refcnt, ==, 1);
> >> > >      g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >> > >
> >> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> >> > >      const int64_t value = 0x1234567890abcdefLL;
> >> > >
> >> > >      qn = qnum_from_int(value);
> >> > > -    g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> >> > > +    g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> >> > >
> >> > >      qobject_unref(qn);
> >> > >  }
> >> > > --
> >> > > 2.28.0
> >> > >
> >> > >
> >> > >
> >> > lgtm otherwise
> >>
> >> Thanks!
> >>
> >> --
> >> Eduardo
> >>
> >>

On Thu, Nov 19, 2020 at 01:21:58PM -0500, Eduardo Habkost wrote:
> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
[...]
> > >> > > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> > 
> > No space between between (type) and its operand, please.
> > 
> > Could we lift the cast into the macro somehow?
> 
> I think we can.  I had thought the cast in the macro would break
> usage as static variable initializers.  I was wrong.

Actually, including the cast in the macro breaks QLIT_QDICT
initializers (which use (QLitDictEntry[]) compound literals), and
I don't know why.

Compound literals in initializers of static variables is a GCC
extension.  I don't understand why it doesn't work inside array
compound literals, though.

Any language lawyers around?

This works:

  typedef struct QLit {
      int x, y;
  } QLit;
  
  typedef struct Entry {
      int key;
      QLit value;
  } Entry;
  
  Entry e = { .key = 0, .value = (QLit) { 1,   2 } };

This works:

  Entry *es1 = (Entry[]) {
      { .key = 0, .value = { 1,   2 } },
  };

But this doesn't:

  Entry *es2 = (Entry[]) {
      { .key = 0, .value = (QLit) { 1,   2 } },
  };

dict.c:16:24: error: initializer element is not constant
   16 | Entry *es2 = (Entry[]) {
      |                        ^
dict.c:16:24: note: (near initialization for ‘es2’)

(gcc (GCC) 10.2.1 20201005 (Red Hat 10.2.1-5))

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> 
>> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >
>> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
>> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
>> >> wrote:
>> >> >
>> >> > > Provide a separate QNumValue type that can be used for QNum value
>> >> > > literals without the referencing counting and memory allocation
>> >> > > features provided by QObject.
>> >> > >
>> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> > > ---
>> >> > > Changes v1 -> v2:
>> >> > > * Fix "make check" failure, by updating check-qnum unit test to
>> >> > >   use the new struct fields
>> >> > > ---
>> >> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
>> >> > >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
>> >> > >  tests/check-qnum.c      | 14 ++++----
>> >> > >  3 files changed, 84 insertions(+), 48 deletions(-)
>> >> > >
>> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
>> >> > > index 55c27b1c24..62fbdfda68 100644
>> >> > > --- a/include/qapi/qmp/qnum.h
>> >> > > +++ b/include/qapi/qmp/qnum.h
>> >> > > @@ -46,20 +46,56 @@ typedef enum {
>> >> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>> >> > >   * convert under the hood.
>> >> > >   */
>> >> > > -struct QNum {
>> >> > > -    struct QObjectBase_ base;
>> >> > > +
>> >> > > +/**
>> >> > > + * struct QNumValue: the value of a QNum
>> >> > > + *
>> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
>> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
>> >> > > + */
>> >> > > +typedef struct QNumValue {
>> >> > > +    /* private: */
>> 
>> Do we care?
>
> Are you asking if we want to make them private, or if we want to
> document them as private (assuming they are).
>
> The answer to the latter is yes ("private:" is an indication to
> kernel-doc).  The answer to the former seems to be "no", based on
> your other comments.
>
> Or maybe `kind` should be public and `u` should be private?

You're factoring out a part of struct QNum into new struct QNumValue.
struct QNum is not private before the patch.  I see no need to start
making it or parts of it private now.

When the structure of a data type is to be kept away from its users, I
prefer to keep it out of the public header, so the compiler enforces the
encapsulation.

[...]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Nov 19, 2020 at 01:21:58PM -0500, Eduardo Habkost wrote:
>> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> [...]
>> > >> > > +    return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
>> > 
>> > No space between between (type) and its operand, please.
>> > 
>> > Could we lift the cast into the macro somehow?
>> 
>> I think we can.  I had thought the cast in the macro would break
>> usage as static variable initializers.  I was wrong.
>
> Actually, including the cast in the macro breaks QLIT_QDICT
> initializers (which use (QLitDictEntry[]) compound literals), and
> I don't know why.
>
> Compound literals in initializers of static variables is a GCC
> extension.  I don't understand why it doesn't work inside array
> compound literals, though.
>
> Any language lawyers around?
>
> This works:
>
>   typedef struct QLit {
>       int x, y;
>   } QLit;
>   
>   typedef struct Entry {
>       int key;
>       QLit value;
>   } Entry;
>   
>   Entry e = { .key = 0, .value = (QLit) { 1,   2 } };
>
> This works:
>
>   Entry *es1 = (Entry[]) {
>       { .key = 0, .value = { 1,   2 } },
>   };
>
> But this doesn't:
>
>   Entry *es2 = (Entry[]) {
>       { .key = 0, .value = (QLit) { 1,   2 } },
>   };
>
> dict.c:16:24: error: initializer element is not constant
>    16 | Entry *es2 = (Entry[]) {
>       |                        ^
> dict.c:16:24: note: (near initialization for ‘es2’)
>
> (gcc (GCC) 10.2.1 20201005 (Red Hat 10.2.1-5))

Can't explain this offhand.

Another pecularity: a const QLitObject is for the most part not actually
const.  Evidence:

    $ size bld-x86/libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-introspect.c.o
       text	   data	    bss	    dec	    hex	filename
      19590	 351600	     48	 371238	  5aa26	bld-x86/libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-introspect.c.o

Score 5 out of 100 points.

On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >> 
> >> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> >
> >> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> >> >> wrote:
> >> >> >
> >> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> >> > > literals without the referencing counting and memory allocation
> >> >> > > features provided by QObject.
> >> >> > >
> >> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> >> > > ---
> >> >> > > Changes v1 -> v2:
> >> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> >> > >   use the new struct fields
> >> >> > > ---
> >> >> > >  include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >> >> > >  qobject/qnum.c          | 78 ++++++++++++++++++++---------------------
> >> >> > >  tests/check-qnum.c      | 14 ++++----
> >> >> > >  3 files changed, 84 insertions(+), 48 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> >> > > index 55c27b1c24..62fbdfda68 100644
> >> >> > > --- a/include/qapi/qmp/qnum.h
> >> >> > > +++ b/include/qapi/qmp/qnum.h
> >> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> >> > >   * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >> >> > >   * convert under the hood.
> >> >> > >   */
> >> >> > > -struct QNum {
> >> >> > > -    struct QObjectBase_ base;
> >> >> > > +
> >> >> > > +/**
> >> >> > > + * struct QNumValue: the value of a QNum
> >> >> > > + *
> >> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> >> > > + */
> >> >> > > +typedef struct QNumValue {
> >> >> > > +    /* private: */
> >> 
> >> Do we care?
> >
> > Are you asking if we want to make them private, or if we want to
> > document them as private (assuming they are).
> >
> > The answer to the latter is yes ("private:" is an indication to
> > kernel-doc).  The answer to the former seems to be "no", based on
> > your other comments.
> >
> > Or maybe `kind` should be public and `u` should be private?
> 
> You're factoring out a part of struct QNum into new struct QNumValue.
> struct QNum is not private before the patch.  I see no need to start
> making it or parts of it private now.

I don't want to change the rules, just to document the existing
implicit rules.  If you say QNum.u was never private, I won't
argue.

> 
> When the structure of a data type is to be kept away from its users, I
> prefer to keep it out of the public header, so the compiler enforces the
> encapsulation.

I prefer that too, except that it is impossible when users of the
API need the compiler to know the struct size.

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
[...]
>> When the structure of a data type is to be kept away from its users, I
>> prefer to keep it out of the public header, so the compiler enforces the
>> encapsulation.
>
> I prefer that too, except that it is impossible when users of the
> API need the compiler to know the struct size.

There are cases where the structure of a data type should be
encapsulated, yet its size must be made known for performance (avoid
dynamic memory allocation and pointer chasing).

Need for encapsulation correlates with complex algorithms and data
structures.  The cost of dynamic allocation is often in the noise then.

On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> [...]
> >> When the structure of a data type is to be kept away from its users, I
> >> prefer to keep it out of the public header, so the compiler enforces the
> >> encapsulation.
> >
> > I prefer that too, except that it is impossible when users of the
> > API need the compiler to know the struct size.
> 
> There are cases where the structure of a data type should be
> encapsulated, yet its size must be made known for performance (avoid
> dynamic memory allocation and pointer chasing).
> 
> Need for encapsulation correlates with complex algorithms and data
> structures.  The cost of dynamic allocation is often in the noise then.

I don't know what we are talking about anymore.  None of this
applies to the QNum API, right?

QNum/QNumValue are not complex data structures, and the reason we
need the compiler to know the size of QNumValue is not related to
performance at all.

We might still want to discourage users of the QNum API from
accessing QNum.u/QNumValue.u directly.  Documenting the field as
private is a very easy way to do it.

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> [...]
>> >> When the structure of a data type is to be kept away from its users, I
>> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> encapsulation.
>> >
>> > I prefer that too, except that it is impossible when users of the
>> > API need the compiler to know the struct size.
>> 
>> There are cases where the structure of a data type should be
>> encapsulated, yet its size must be made known for performance (avoid
>> dynamic memory allocation and pointer chasing).
>> 
>> Need for encapsulation correlates with complex algorithms and data
>> structures.  The cost of dynamic allocation is often in the noise then.
>
> I don't know what we are talking about anymore.  None of this
> applies to the QNum API, right?
>
> QNum/QNumValue are not complex data structures, and the reason we
> need the compiler to know the size of QNumValue is not related to
> performance at all.

We started with the question whether to make QNumValue's members
private.  We digressed to the question when to make members private.
So back to the original question.

> We might still want to discourage users of the QNum API from
> accessing QNum.u/QNumValue.u directly.  Documenting the field as
> private is a very easy way to do it.

It's a complete non-issue.  QNum has been around for years, and we
haven't had any issues that could've been plausibly avoided by asking
people to refrain from accessing its members.

If there was an actual need to keep the members private, I'd move the
struct out of the header, so the compiler enforces privacy.

On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> [...]
> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> encapsulation.
> >> >
> >> > I prefer that too, except that it is impossible when users of the
> >> > API need the compiler to know the struct size.
> >> 
> >> There are cases where the structure of a data type should be
> >> encapsulated, yet its size must be made known for performance (avoid
> >> dynamic memory allocation and pointer chasing).
> >> 
> >> Need for encapsulation correlates with complex algorithms and data
> >> structures.  The cost of dynamic allocation is often in the noise then.
> >
> > I don't know what we are talking about anymore.  None of this
> > applies to the QNum API, right?
> >
> > QNum/QNumValue are not complex data structures, and the reason we
> > need the compiler to know the size of QNumValue is not related to
> > performance at all.
> 
> We started with the question whether to make QNumValue's members
> private.  We digressed to the question when to make members private.
> So back to the original question.
> 
> > We might still want to discourage users of the QNum API from
> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
> > private is a very easy way to do it.
> 
> It's a complete non-issue.  QNum has been around for years, and we
> haven't had any issues that could've been plausibly avoided by asking
> people to refrain from accessing its members.
> 
> If there was an actual need to keep the members private, I'd move the
> struct out of the header, so the compiler enforces privacy.

Understood.  There's still a question I'd like to answer, to
decide how the API documentation should look like:

  Is QNum.u/QNumValue.u required to be part of the API
  documentation?

If accessing that field directly is not necessary for using the
API, I don't think it should appear in the documentation (because
it would be just noise).

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> >> [...]
>> >> >> When the structure of a data type is to be kept away from its users, I
>> >> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> >> encapsulation.
>> >> >
>> >> > I prefer that too, except that it is impossible when users of the
>> >> > API need the compiler to know the struct size.
>> >> 
>> >> There are cases where the structure of a data type should be
>> >> encapsulated, yet its size must be made known for performance (avoid
>> >> dynamic memory allocation and pointer chasing).
>> >> 
>> >> Need for encapsulation correlates with complex algorithms and data
>> >> structures.  The cost of dynamic allocation is often in the noise then.
>> >
>> > I don't know what we are talking about anymore.  None of this
>> > applies to the QNum API, right?
>> >
>> > QNum/QNumValue are not complex data structures, and the reason we
>> > need the compiler to know the size of QNumValue is not related to
>> > performance at all.
>> 
>> We started with the question whether to make QNumValue's members
>> private.  We digressed to the question when to make members private.
>> So back to the original question.
>> 
>> > We might still want to discourage users of the QNum API from
>> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
>> > private is a very easy way to do it.
>> 
>> It's a complete non-issue.  QNum has been around for years, and we
>> haven't had any issues that could've been plausibly avoided by asking
>> people to refrain from accessing its members.
>> 
>> If there was an actual need to keep the members private, I'd move the
>> struct out of the header, so the compiler enforces privacy.
>
> Understood.  There's still a question I'd like to answer, to
> decide how the API documentation should look like:
>
>   Is QNum.u/QNumValue.u required to be part of the API
>   documentation?
>
> If accessing that field directly is not necessary for using the
> API, I don't think it should appear in the documentation (because
> it would be just noise).

The current patch's comment on QNumValue looks good to me.

Does this answer your question?

On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >> 
> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> >> [...]
> >> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> >> encapsulation.
> >> >> >
> >> >> > I prefer that too, except that it is impossible when users of the
> >> >> > API need the compiler to know the struct size.
> >> >> 
> >> >> There are cases where the structure of a data type should be
> >> >> encapsulated, yet its size must be made known for performance (avoid
> >> >> dynamic memory allocation and pointer chasing).
> >> >> 
> >> >> Need for encapsulation correlates with complex algorithms and data
> >> >> structures.  The cost of dynamic allocation is often in the noise then.
> >> >
> >> > I don't know what we are talking about anymore.  None of this
> >> > applies to the QNum API, right?
> >> >
> >> > QNum/QNumValue are not complex data structures, and the reason we
> >> > need the compiler to know the size of QNumValue is not related to
> >> > performance at all.
> >> 
> >> We started with the question whether to make QNumValue's members
> >> private.  We digressed to the question when to make members private.
> >> So back to the original question.
> >> 
> >> > We might still want to discourage users of the QNum API from
> >> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
> >> > private is a very easy way to do it.
> >> 
> >> It's a complete non-issue.  QNum has been around for years, and we
> >> haven't had any issues that could've been plausibly avoided by asking
> >> people to refrain from accessing its members.
> >> 
> >> If there was an actual need to keep the members private, I'd move the
> >> struct out of the header, so the compiler enforces privacy.
> >
> > Understood.  There's still a question I'd like to answer, to
> > decide how the API documentation should look like:
> >
> >   Is QNum.u/QNumValue.u required to be part of the API
> >   documentation?
> >
> > If accessing that field directly is not necessary for using the
> > API, I don't think it should appear in the documentation (because
> > it would be just noise).
> 
> The current patch's comment on QNumValue looks good to me.
> 
> Does this answer your question?

The current patch (v3) doesn't address the question.  It doesn't
include documentation for the field, but doesn't hide it.
kernel-doc will print a warning on that case.

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> >> 
>> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> >> >> [...]
>> >> >> >> When the structure of a data type is to be kept away from its users, I
>> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> >> >> encapsulation.
>> >> >> >
>> >> >> > I prefer that too, except that it is impossible when users of the
>> >> >> > API need the compiler to know the struct size.
>> >> >> 
>> >> >> There are cases where the structure of a data type should be
>> >> >> encapsulated, yet its size must be made known for performance (avoid
>> >> >> dynamic memory allocation and pointer chasing).
>> >> >> 
>> >> >> Need for encapsulation correlates with complex algorithms and data
>> >> >> structures.  The cost of dynamic allocation is often in the noise then.
>> >> >
>> >> > I don't know what we are talking about anymore.  None of this
>> >> > applies to the QNum API, right?
>> >> >
>> >> > QNum/QNumValue are not complex data structures, and the reason we
>> >> > need the compiler to know the size of QNumValue is not related to
>> >> > performance at all.
>> >> 
>> >> We started with the question whether to make QNumValue's members
>> >> private.  We digressed to the question when to make members private.
>> >> So back to the original question.
>> >> 
>> >> > We might still want to discourage users of the QNum API from
>> >> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
>> >> > private is a very easy way to do it.
>> >> 
>> >> It's a complete non-issue.  QNum has been around for years, and we
>> >> haven't had any issues that could've been plausibly avoided by asking
>> >> people to refrain from accessing its members.
>> >> 
>> >> If there was an actual need to keep the members private, I'd move the
>> >> struct out of the header, so the compiler enforces privacy.
>> >
>> > Understood.  There's still a question I'd like to answer, to
>> > decide how the API documentation should look like:
>> >
>> >   Is QNum.u/QNumValue.u required to be part of the API
>> >   documentation?
>> >
>> > If accessing that field directly is not necessary for using the
>> > API, I don't think it should appear in the documentation (because
>> > it would be just noise).
>> 
>> The current patch's comment on QNumValue looks good to me.
>> 
>> Does this answer your question?
>
> The current patch (v3) doesn't address the question.  It doesn't
> include documentation for the field, but doesn't hide it.
> kernel-doc will print a warning on that case.

Do we care?  How many such warnings exist before the patch?  Does this
series add just this one, or more?

Use your judgement, then be ready to explain it :)

On Wed, Nov 25, 2020 at 07:40:48AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >> 
> >> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >> >> 
> >> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> >> >> [...]
> >> >> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> >> >> encapsulation.
> >> >> >> >
> >> >> >> > I prefer that too, except that it is impossible when users of the
> >> >> >> > API need the compiler to know the struct size.
> >> >> >> 
> >> >> >> There are cases where the structure of a data type should be
> >> >> >> encapsulated, yet its size must be made known for performance (avoid
> >> >> >> dynamic memory allocation and pointer chasing).
> >> >> >> 
> >> >> >> Need for encapsulation correlates with complex algorithms and data
> >> >> >> structures.  The cost of dynamic allocation is often in the noise then.
> >> >> >
> >> >> > I don't know what we are talking about anymore.  None of this
> >> >> > applies to the QNum API, right?
> >> >> >
> >> >> > QNum/QNumValue are not complex data structures, and the reason we
> >> >> > need the compiler to know the size of QNumValue is not related to
> >> >> > performance at all.
> >> >> 
> >> >> We started with the question whether to make QNumValue's members
> >> >> private.  We digressed to the question when to make members private.
> >> >> So back to the original question.
> >> >> 
> >> >> > We might still want to discourage users of the QNum API from
> >> >> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
> >> >> > private is a very easy way to do it.
> >> >> 
> >> >> It's a complete non-issue.  QNum has been around for years, and we
> >> >> haven't had any issues that could've been plausibly avoided by asking
> >> >> people to refrain from accessing its members.
> >> >> 
> >> >> If there was an actual need to keep the members private, I'd move the
> >> >> struct out of the header, so the compiler enforces privacy.
> >> >
> >> > Understood.  There's still a question I'd like to answer, to
> >> > decide how the API documentation should look like:
> >> >
> >> >   Is QNum.u/QNumValue.u required to be part of the API
> >> >   documentation?
> >> >
> >> > If accessing that field directly is not necessary for using the
> >> > API, I don't think it should appear in the documentation (because
> >> > it would be just noise).
> >> 
> >> The current patch's comment on QNumValue looks good to me.
> >> 
> >> Does this answer your question?
> >
> > The current patch (v3) doesn't address the question.  It doesn't
> > include documentation for the field, but doesn't hide it.
> > kernel-doc will print a warning on that case.
> 
> Do we care?

Yes.  Peter will reject pull requests if it generates kernel-doc
warnings.

> How many such warnings exist before the patch?

Zero.

> Does this series add just this one, or more?

The current series (v3) doesn't add any, because I dropped the
patch that added QObject and QNum documentation to docs/devel.  I
still want to resubmit that patch later, though.

> 
> Use your judgement, then be ready to explain it :)

OK!