Re: [PATCH v2 5/8] qlit: Support all types of QNums

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Nov 19, 2020 at 11:39:14AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> Use QNumValue to represent QNums, so we can also support uint64_t
> >> and double QNum values.  Add new QLIT_QNUM_(INT|UINT|DOUBLE)
> >> macros for each case.
> >>
> >> The QLIT_QNUM() macro is being kept for compatibility with
> >> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
> >>
> >
> > I am not sure it's worth to keep. (furthermore, it's only used in tests
> > afaics)
> 
> Seconded.

Understood, I will remove it.  I thought the QAPI code generator
was using it.

[...]
> >> diff --git a/qobject/qlit.c b/qobject/qlit.c
> >> index be8332136c..b23cdc4532 100644
> >> --- a/qobject/qlit.c
> >> +++ b/qobject/qlit.c
> >> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> >> QObject *rhs)
> >>      case QTYPE_QBOOL:
> >>          return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
> >>      case QTYPE_QNUM:
> >> -        return lhs->value.qnum ==  qnum_get_int(qobject_to(QNum, rhs));
> >> +        return qnum_value_is_equal(&lhs->value.qnum,
> >> +                                   qnum_get_value(qobject_to(QNum, rhs)));
> 
> Before the patch, we crash when @rhs can't be represented as int64_t.

I thought it was expected behavior?  QLit never supported
QNUM_U64 or QNUM_DOUBLE as input.

> 
> Afterwards, we return false (I think).
> 
> Please note this in the commit message.  A separate fix preceding this
> patch would be even better, but may not be worth the trouble.  Up to
> you.

The fix would be 3 or 4 extra lines of code that would be
immediately deleted.  I'll just mention it as a side effect of
the new feature.

> 
> >>      case QTYPE_QSTRING:
> >>          return (strcmp(lhs->value.qstr,
> >>                         qstring_get_str(qobject_to(QString, rhs))) == 0);
> >> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
> >>      case QTYPE_QNULL:
> >>          return QOBJECT(qnull());
> >>      case QTYPE_QNUM:
> >> -        return QOBJECT(qnum_from_int(qlit->value.qnum));
> >> +        return QOBJECT(qnum_from_value(qlit->value.qnum));
> >>      case QTYPE_QSTRING:
> >>          return QOBJECT(qstring_from_str(qlit->value.qstr));
> >>      case QTYPE_QDICT: {
> >> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> >> index 07a773e653..711030cffd 100644
> >> --- a/tests/check-qjson.c
> >> +++ b/tests/check-qjson.c
> >> @@ -796,20 +796,23 @@ static void simple_number(void)
> >>      int i;
> >>      struct {
> >>          const char *encoded;
> >> +        QLitObject qlit;
> >>          int64_t decoded;
> >>          int skip;
> >>      } test_cases[] = {
> >> -        { "0", 0 },
> >> -        { "1234", 1234 },
> >> -        { "1", 1 },
> >> -        { "-32", -32 },
> >> -        { "-0", 0, .skip = 1 },
> >> +        { "0",    QLIT_QNUM(0),    0, },
> >> +        { "1234", QLIT_QNUM(1234), 1234, },
> >> +        { "1",    QLIT_QNUM(1),    1, },
> >> +        { "-32",  QLIT_QNUM(-32),  -32, },
> >> +        { "-0",   QLIT_QNUM(0),    0, .skip = 1 },
> 
> Note .qlit is always QLIT_QNUM(.decoded).  Would doing without .qlit
> result in a simpler patch?

Good point.  When I wrote this, I mistakenly thought we would end
up having different types of qlits in the array.

I still want to test multiple types of QNums here, not just
QNUM_I64.  I will try to get something that is simple but also
gets us more coverage.  Maybe as a separate test function and/or
a separate patch.

> 
> >>          { },
> >>      };
> >>
> >>      for (i = 0; test_cases[i].encoded; i++) {
> >>          QNum *qnum;
> >>          int64_t val;
> >> +        QNum *qlit_num;
> >> +        int64_t qlit_val;
> >>
> >>          qnum = qobject_to(QNum,
> >>                            qobject_from_json(test_cases[i].encoded,
> >> @@ -817,6 +820,7 @@ static void simple_number(void)
> >>          g_assert(qnum);
> >>          g_assert(qnum_get_try_int(qnum, &val));
> >>          g_assert_cmpint(val, ==, test_cases[i].decoded);
> >> +
> >>          if (test_cases[i].skip == 0) {
> >>              QString *str;
> >>
> >> @@ -826,9 +830,66 @@ static void simple_number(void)
> >>          }
> >>
> >>          qobject_unref(qnum);
> >> +
> >> +        qlit_num = qobject_to(QNum,
> >> +                              qobject_from_qlit(&test_cases[i].qlit));
> >> +        g_assert(qlit_num);
> >> +        g_assert(qnum_get_try_int(qlit_num, &qlit_val));
> >> +        g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
> >> +
> >> +        qobject_unref(qlit_num);
> >>      }
> >>  }
> >>
[...]

-- 
Eduardo