[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to us
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to use QNUM_U64 |
Date: |
Fri, 02 Jun 2017 14:36:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> ----- Original Message -----
>> One more nitpick:
>>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > Switch to use QNum/uint where appropriate to remove i64 limitation.
>> >
>> > The input visitor will cast i64 input to u64 for compatibility
>> > reasons (existing json QMP client already use negative i64 for large
>> > u64, and expect an implicit cast in qemu).
>> >
>> > Signed-off-by: Marc-André Lureau <address@hidden>
>> [...]
>> > diff --git a/tests/test-qobject-output-visitor.c
>> > b/tests/test-qobject-output-visitor.c
>> > index 3180d8cbde..d9f106d52e 100644
>> > --- a/tests/test-qobject-output-visitor.c
>> > +++ b/tests/test-qobject-output-visitor.c
>> > @@ -602,17 +602,31 @@ static void check_native_list(QObject *qobj,
>> > qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data")));
>> >
>> > switch (kind) {
>> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
>> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
>> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
>> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
>> > case USER_DEF_NATIVE_LIST_UNION_KIND_U8:
>> > case USER_DEF_NATIVE_LIST_UNION_KIND_U16:
>> > case USER_DEF_NATIVE_LIST_UNION_KIND_U32:
>> > case USER_DEF_NATIVE_LIST_UNION_KIND_U64:
>> > - /* all integer elements in JSON arrays get stored into QNums when
>> > - * we convert to QObjects, so we can check them all in the same
>> > - * fashion, so simply fall through here
>> > + for (i = 0; i < 32; i++) {
>> > + QObject *tmp;
>> > + QNum *qvalue;
>> > + uint64_t val;
>> > +
>> > + tmp = qlist_peek(qlist);
>> > + g_assert(tmp);
>> > + qvalue = qobject_to_qnum(tmp);
>> > + g_assert(qnum_get_uint(qvalue, &val));
>> > + g_assert_cmpuint(val, ==, i);
>> > + qobject_decref(qlist_pop(qlist));
>> > + }
>> > + break;
>> > +
>> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
>> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
>> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
>> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
>> > + /* All signed integer elements in JSON arrays get stored into
>> > + * QInts when we convert to QObjects, so we can check them all
>> > + * in the same fashion, so simply fall through here.
>> > */
>> > case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER:
>> > for (i = 0; i < 32; i++) {
>>
>> Wing both ends of the comment, please.
>
> You want this: ?
>
> /* All signed integer elements in JSON arrays get stored into */
> /* QInts when we convert to QObjects, so we can check them all */
> ...
Ewww!
> I think this is quite annoying for refilling with emacs
Yup.
> Perhaps rather
>
> /* All signed integer elements in JSON arrays get stored into
> * QInts when we convert to QObjects, so we can check them all */
>
> quite uglier to me, but I don't care
I'm asking for this:
/*
* All signed integer elements in JSON arrays get stored into
* QInts when we convert to QObjects, so we can check them all
* in the same fashion, so simply fall through here.
*/
This comment style is commonly called "winged". It's fairly widespread
in QEMU.
> However, I'd prefer if either we have a common rule in qemu or we don't
> bike-sched over that...
>
> (even better would be to have this somehow automated with tools like
> git-clang-format)
Tool support would be easier if we didn't insist on inventing our very
own coding style, then fail to enforce it uniformly. Oh well, we'll
muddle on.