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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

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

Yes.  It's a known limitation, not a bug.  You're lifting the
limitation, and that's worth noting in the commit message.

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

That's okay.

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

Use your judgement :)

[...]