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

[Markus Armbruster]

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.

>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
>> Changes v1 -> v2:
>> * Coding style fix at qlit_equal_qobject()
>> ---
>>  include/qapi/qmp/qlit.h | 11 +++++--
>>  qobject/qlit.c          |  5 +--
>>  tests/check-qjson.c     | 72 ++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 79 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
>> index c0676d5daf..f9e356d31e 100644
>> --- a/include/qapi/qmp/qlit.h
>> +++ b/include/qapi/qmp/qlit.h
>> @@ -15,6 +15,7 @@
>>  #define QLIT_H
>>
>>  #include "qobject.h"
>> +#include "qnum.h"
>>
>>  typedef struct QLitDictEntry QLitDictEntry;
>>  typedef struct QLitObject QLitObject;
>> @@ -23,7 +24,7 @@ struct QLitObject {
>>      QType type;
>>      union {
>>          bool qbool;
>> -        int64_t qnum;
>> +        QNumValue qnum;
>>          const char *qstr;
>>          QLitDictEntry *qdict;
>>          QLitObject *qlist;
>> @@ -39,8 +40,14 @@ struct QLitDictEntry {
>>      { .type = QTYPE_QNULL }
>>  #define QLIT_QBOOL(val) \
>>      { .type = QTYPE_QBOOL, .value.qbool = (val) }
>> +#define QLIT_QNUM_INT(val) \
>> +    { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
>> +#define QLIT_QNUM_UINT(val) \
>> +    { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
>> +#define QLIT_QNUM_DOUBLE(val) \
>> +    { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
>>  #define QLIT_QNUM(val) \
>> -    { .type = QTYPE_QNUM, .value.qnum = (val) }
>> +    QLIT_QNUM_INT(val)
>>  #define QLIT_QSTR(val) \
>>      { .type = QTYPE_QSTRING, .value.qstr = (val) }
>>  #define QLIT_QDICT(val) \
>> 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.

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.

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

>>          { },
>>      };
>>
>>      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);
>>      }
>>  }
>>
>> +static void qlit_large_number(void)
>> +{
>> +    QLitObject maxu64 = QLIT_QNUM_UINT(UINT64_MAX);
>> +    QLitObject maxi64 = QLIT_QNUM(INT64_MAX);
>> +    QLitObject mini64 = QLIT_QNUM(INT64_MIN);
>> +    QLitObject gtu64  = QLIT_QNUM_DOUBLE(18446744073709552e3);
>> +    QLitObject lti64  = QLIT_QNUM_DOUBLE(-92233720368547758e2);
>> +    QNum *qnum;
>> +    uint64_t val;
>> +    int64_t ival;
>> +
>> +    qnum = qobject_to(QNum, qobject_from_qlit(&maxu64));
>> +    g_assert(qnum);
>> +    g_assert_cmpuint(qnum_get_uint(qnum), ==, UINT64_MAX);
>> +    g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> +    qobject_unref(qnum);
>> +
>> +    qnum = qobject_to(QNum, qobject_from_qlit(&maxi64));
>> +    g_assert(qnum);
>> +    g_assert_cmpuint(qnum_get_uint(qnum), ==, INT64_MAX);
>> +    g_assert_cmpint(qnum_get_int(qnum), ==, INT64_MAX);
>> +
>> +    qobject_unref(qnum);
>> +
>> +    qnum = qobject_to(QNum, qobject_from_qlit(&mini64));
>> +    g_assert(qnum);
>> +    g_assert(!qnum_get_try_uint(qnum, &val));
>> +    g_assert_cmpuint(qnum_get_int(qnum), ==, INT64_MIN);
>> +
>> +    qobject_unref(qnum);
>> +
>> +    qnum = qobject_to(QNum, qobject_from_qlit(&gtu64));
>> +    g_assert(qnum);
>> +    g_assert_cmpfloat(qnum_get_double(qnum), ==, 18446744073709552e3);
>> +    g_assert(!qnum_get_try_uint(qnum, &val));
>> +    g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> +    qobject_unref(qnum);
>> +
>> +    qnum = qobject_to(QNum, qobject_from_qlit(&lti64));
>> +    g_assert(qnum);
>> +    g_assert_cmpfloat(qnum_get_double(qnum), ==, -92233720368547758e2);
>> +    g_assert(!qnum_get_try_uint(qnum, &val));
>> +    g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> +    qobject_unref(qnum);
>> +}
>> +
>>  static void large_number(void)
>>  {
>>      const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
>> @@ -1472,6 +1533,7 @@ int main(int argc, char **argv)
>>      g_test_add_func("/literals/string/utf8", utf8_string);
>>
>>      g_test_add_func("/literals/number/simple", simple_number);
>> +    g_test_add_func("/literals/number/qlit_large", qlit_large_number);
>>      g_test_add_func("/literals/number/large", large_number);
>>      g_test_add_func("/literals/number/float", float_number);
>>
>> --
>> 2.28.0
>>
>>
>>