Re: [Qemu-devel] [PATCH v2 13/14] qlit: add qobject_form_qlit()

[Marc-André Lureau]

Hi

----- Original Message -----
> Eric already pointed out the typo in the title.
> 
> Marc-André Lureau <address@hidden> writes:
> 
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  include/qapi/qmp/qlit.h |  2 ++
> >  qobject/qlit.c          | 37 +++++++++++++++++++++++++++++++++++++
> >  tests/check-qlit.c      | 26 ++++++++++++++++++++++++++
> >  3 files changed, 65 insertions(+)
> >
> > diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> > index b18406bce9..56feb25e04 100644
> > --- a/include/qapi/qmp/qlit.h
> > +++ b/include/qapi/qmp/qlit.h
> > @@ -51,4 +51,6 @@ struct QLitDictEntry {
> >  
> >  bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs);
> >  
> > +QObject *qobject_from_qlit(const QLitObject *qlit);
> > +
> >  #endif /* QLIT_H */
> > diff --git a/qobject/qlit.c b/qobject/qlit.c
> > index 3c4882c784..e44c09e9eb 100644
> > --- a/qobject/qlit.c
> > +++ b/qobject/qlit.c
> > @@ -82,3 +82,40 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> > QObject *rhs)
> >  
> >      return false;
> >  }
> > +
> > +QObject *qobject_from_qlit(const QLitObject *qlit)
> > +{
> > +    int i;
> > +
> > +    switch (qlit->type) {
> > +    case QTYPE_QNULL:
> > +        return QOBJECT(qnull());
> > +    case QTYPE_QNUM:
> > +        return QOBJECT(qnum_from_int(qlit->value.qnum));
> 
> This line shows that QLitObject member value.qnum should really be named
> value.qint.  Let's not worry about that now.
> 
> > +    case QTYPE_QSTRING:
> > +        return QOBJECT(qstring_from_str(qlit->value.qstr));
> > +    case QTYPE_QDICT: {
> > +        QDict *qdict = qdict_new();
> 
> Blank line here.  Can touch up when I apply.
> 
> > +        for (i = 0; qlit->value.qdict[i].value.type != QTYPE_NONE; i++) {
> > +            QLitDictEntry *e = &qlit->value.qdict[i];
> 
> I'd prefer the laconic
> 
>         QDict *qdict = qdict_new();
>         QLitDictEntry *e;
> 
>         for (e = qlit->value.qdict; e->key; e++) {
>             qdict_put_obj(qdict, e->key, qobject_from_qlit(&e->value));
>         }
>         return QOBJECT(qdict);
> 
> Can slot that in when I apply.
> 

Indeed!

> > +
> > +            qdict_put_obj(qdict, e->key, qobject_from_qlit(&e->value));
> > +        }
> > +        return QOBJECT(qdict);
> > +    }
> > +    case QTYPE_QLIST: {
> > +        QList *qlist = qlist_new();
> > +
> > +        for (i = 0; qlit->value.qlist[i].type != QTYPE_NONE; i++) {
> > +            qlist_append_obj(qlist,
> > qobject_from_qlit(&qlit->value.qlist[i]));
> > +        }
> > +        return QOBJECT(qlist);
> 
> Likewise
> 
>         QList *qlist = qlist_new();
>         QLitObject *e;
> 
>         for (e = qlit->value.qlist; e->type != QTYPE_NONE; e++) {
>             qlist_append_obj(qlist, qobject_from_qlit(e));
>         }
>         return QOBJECT(qlist);
> 
> > +    }
> > +    case QTYPE_QBOOL:
> > +        return QOBJECT(qbool_from_bool(qlit->value.qbool));
> > +    case QTYPE_NONE:
> > +        assert(0);
> > +    }
> > +
> > +    return NULL;
> > +}
> > diff --git a/tests/check-qlit.c b/tests/check-qlit.c
> > index ae74bfcb83..135a399c6d 100644
> > --- a/tests/check-qlit.c
> > +++ b/tests/check-qlit.c
> > @@ -64,11 +64,37 @@ static void qlit_equal_qobject_test(void)
> >      qobject_decref(qobj);
> >  }
> >  
> > +static void qobject_from_qlit_test(void)
> > +{
> > +    QObject *obj, *qobj = qobject_from_qlit(&qlit);
> > +    QDict *qdict;
> > +    QList *bee;
> > +
> > +    qdict = qobject_to_qdict(qobj);
> > +    g_assert_cmpint(qdict_get_int(qdict, "foo"), ==, 42);
> > +    g_assert_cmpstr(qdict_get_str(qdict, "bar"), ==, "hello world");
> > +    g_assert(qobject_type(qdict_get(qdict, "baz")) == QTYPE_QNULL);
> > +
> > +    bee = qdict_get_qlist(qdict, "bee");
> > +    obj = qlist_pop(bee);
> > +    g_assert_cmpint(qnum_get_int(qobject_to_qnum(obj)), ==, 43);
> > +    qobject_decref(obj);
> > +    obj = qlist_pop(bee);
> > +    g_assert_cmpint(qnum_get_int(qobject_to_qnum(obj)), ==, 44);
> > +    qobject_decref(obj);
> > +    obj = qlist_pop(bee);
> > +    g_assert(qbool_get_bool(qobject_to_qbool(obj)));
> > +    qobject_decref(obj);
> > +
> > +    qobject_decref(qobj);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      g_test_init(&argc, &argv, NULL);
> >  
> >      g_test_add_func("/qlit/equal_qobject", qlit_equal_qobject_test);
> > +    g_test_add_func("/qlit/qobject_from_qlit", qobject_from_qlit_test);
> >  
> >      return g_test_run();
> >  }
> 
> With the typo fixed and preferably with the improvements I suggested:
> Reviewed-by: Markus Armbruster <address@hidden>

Thanks