[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/8] qlit: Support all types of QNums
From: |
Eduardo Habkost |
Subject: |
Re: [PATCH v2 5/8] qlit: Support all types of QNums |
Date: |
Thu, 19 Nov 2020 13:56:04 -0500 |
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
- Re: [PATCH v2 7/8] qom: Make object_property_set_default() public, (continued)
Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults, Markus Armbruster, 2020/11/19