Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to use QNUM_U64

[Markus Armbruster]

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.