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