Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
allowing to statically declare complex qobjects.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/qlit.h |  54 +++++++++++++++++
 qobject/qlit.c          | 106 ++++++++++++++++++++++++++++++++++
 tests/check-qjson.c     | 150 +++++++++---------------------------------------
 tests/check-qlit.c      |  68 ++++++++++++++++++++++
 qobject/Makefile.objs   |   2 +-
 tests/Makefile.include  |   5 +-
 6 files changed, 261 insertions(+), 124 deletions(-)
 create mode 100644 include/qapi/qmp/qlit.h
 create mode 100644 qobject/qlit.c
 create mode 100644 tests/check-qlit.c

diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
new file mode 100644
index 0000000000..2cdceb448d
--- /dev/null
+++ b/include/qapi/qmp/qlit.h
@@ -0,0 +1,54 @@
+/*
+ * Copyright IBM, Corp. 2009
+ * Copyright (c) 2013, 2015, 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Markus Armbruster <armbru@redhat.com>
+ *  Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+#ifndef QLIT_H_
+#define QLIT_H_
+
+#include "qapi-types.h"
+#include "qobject.h"
+
+typedef struct QLitDictEntry QLitDictEntry;
+typedef struct QLitObject QLitObject;
+
+struct QLitObject {
+    int type;
+    union {
+        bool qbool;
+        int64_t qnum;
+        const char *qstr;
+        QLitDictEntry *qdict;
+        QLitObject *qlist;
+    } value;
+};
+
+struct QLitDictEntry {
+    const char *key;
+    QLitObject value;
+};
+
+#define QLIT_QNULL \
+    { .type = QTYPE_QNULL }
+#define QLIT_QBOOL(val) \
+    { .type = QTYPE_QBOOL, .value.qbool = (val) }
+#define QLIT_QNUM(val) \
+    { .type = QTYPE_QNUM, .value.qnum = (val) }
+#define QLIT_QSTR(val) \
+    { .type = QTYPE_QSTRING, .value.qstr = (val) }
+#define QLIT_QDICT(val) \
+    { .type = QTYPE_QDICT, .value.qdict = (val) }
+#define QLIT_QLIST(val) \
+    { .type = QTYPE_QLIST, .value.qlist = (val) }
+
+bool qlit_equal_qobject(QLitObject *lhs, QObject *rhs);
+
+#endif /* QLIT_H_ */
diff --git a/qobject/qlit.c b/qobject/qlit.c
new file mode 100644
index 0000000000..f4ebeb6259
--- /dev/null
+++ b/qobject/qlit.c
@@ -0,0 +1,106 @@
+/*
+ * QLit literal qobject
+ *
+ * Copyright IBM, Corp. 2009
+ * Copyright (c) 2013, 2015, 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Markus Armbruster <armbru@redhat.com>
+ *  Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/qmp/qlit.h"
+#include "qapi/qmp/types.h"
+
+typedef struct QListCompareHelper {
+    int index;
+    QLitObject *objs;
+    bool result;
+} QListCompareHelper;
+
+static void compare_helper(QObject *obj, void *opaque)
+{
+    QListCompareHelper *helper = opaque;
+
+    if (!helper->result) {
+        return;
+    }
+
+    if (helper->objs[helper->index].type == QTYPE_NONE) {
+        helper->result = false;
+        return;
+    }
+
+    helper->result =
+        qlit_equal_qobject(&helper->objs[helper->index++], obj);
+}
+
+bool qlit_equal_qobject(QLitObject *lhs, QObject *rhs)
+{
+    int64_t val;
+
+    if (!rhs || lhs->type != qobject_type(rhs)) {
+        return false;
+    }
+
+    switch (lhs->type) {
+    case QTYPE_QBOOL:
+        return lhs->value.qbool == qbool_get_bool(qobject_to_qbool(rhs));
+    case QTYPE_QNUM:
+        val = qnum_get_int(qobject_to_qnum(rhs));
+        return lhs->value.qnum == val;
+    case QTYPE_QSTRING:
+        return g_str_equal(lhs->value.qstr,
+                           qstring_get_str(qobject_to_qstring(rhs)));
+    case QTYPE_QDICT: {
+        int i;
+
+        for (i = 0; lhs->value.qdict[i].key; i++) {
+            QObject *obj = qdict_get(qobject_to_qdict(rhs),
+                                     lhs->value.qdict[i].key);
+
+            if (!qlit_equal_qobject(&lhs->value.qdict[i].value, obj)) {
+                return false;
+            }
+        }
+
+        if (qdict_size(qobject_to_qdict(rhs)) != i) {
+            return false;
+        }
+
+        return true;
+    }
+    case QTYPE_QLIST: {
+        QListCompareHelper helper;
+        int i;
+
+        for (i = 0; lhs->value.qlist[i].type != QTYPE_NONE; i++) {
+            continue;
+        }
+
+        if (qlist_size(qobject_to_qlist(rhs)) != i) {
+            return false;
+        }
+
+        helper.index = 0;
+        helper.objs = lhs->value.qlist;
+        helper.result = true;
+
+        qlist_iter(qobject_to_qlist(rhs), compare_helper, &helper);
+
+        return helper.result;
+    }
+    case QTYPE_QNULL:
+        return true;
+    default:
+        break;
+    }
+
+    return false;
+}
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index a3a97b0d99..59227934ce 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -16,6 +16,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qlit.h"
 #include "qemu-common.h"
 
 static void escaped_string(void)
@@ -1059,123 +1060,28 @@ static void keyword_literal(void)
     QDECREF(null);
 }
 
-typedef struct LiteralQDictEntry LiteralQDictEntry;
-typedef struct LiteralQObject LiteralQObject;
-
-struct LiteralQObject
-{
-    int type;
-    union {
-        int64_t qnum;
-        const char *qstr;
-        LiteralQDictEntry *qdict;
-        LiteralQObject *qlist;
-    } value;
-};
-
-struct LiteralQDictEntry
-{
-    const char *key;
-    LiteralQObject value;
-};
-
-#define QLIT_QNUM(val) (LiteralQObject){.type = QTYPE_QNUM, .value.qnum = (val)}
-#define QLIT_QSTR(val) (LiteralQObject){.type = QTYPE_QSTRING, .value.qstr = (val)}
-#define QLIT_QDICT(val) (LiteralQObject){.type = QTYPE_QDICT, .value.qdict = (val)}
-#define QLIT_QLIST(val) (LiteralQObject){.type = QTYPE_QLIST, .value.qlist = (val)}
-
-typedef struct QListCompareHelper
-{
-    int index;
-    LiteralQObject *objs;
-    int result;
-} QListCompareHelper;
-
-static int compare_litqobj_to_qobj(LiteralQObject *lhs, QObject *rhs);
-
-static void compare_helper(QObject *obj, void *opaque)
-{
-    QListCompareHelper *helper = opaque;
-
-    if (helper->result == 0) {
-        return;
-    }
-
-    if (helper->objs[helper->index].type == QTYPE_NONE) {
-        helper->result = 0;
-        return;
-    }
-
-    helper->result = compare_litqobj_to_qobj(&helper->objs[helper->index++], obj);
-}
-
-static int compare_litqobj_to_qobj(LiteralQObject *lhs, QObject *rhs)
-{
-    int64_t val;
-
-    if (!rhs || lhs->type != qobject_type(rhs)) {
-        return 0;
-    }
-
-    switch (lhs->type) {
-    case QTYPE_QNUM:
-        g_assert(qnum_get_try_int(qobject_to_qnum(rhs), &val));
-        return lhs->value.qnum == val;
-    case QTYPE_QSTRING:
-        return (strcmp(lhs->value.qstr, qstring_get_str(qobject_to_qstring(rhs))) == 0);
-    case QTYPE_QDICT: {
-        int i;
-
-        for (i = 0; lhs->value.qdict[i].key; i++) {
-            QObject *obj = qdict_get(qobject_to_qdict(rhs), lhs->value.qdict[i].key);
-
-            if (!compare_litqobj_to_qobj(&lhs->value.qdict[i].value, obj)) {
-                return 0;
-            }
-        }
-
-        return 1;
-    }
-    case QTYPE_QLIST: {
-        QListCompareHelper helper;
-
-        helper.index = 0;
-        helper.objs = lhs->value.qlist;
-        helper.result = 1;
-        
-        qlist_iter(qobject_to_qlist(rhs), compare_helper, &helper);
-
-        return helper.result;
-    }
-    default:
-        break;
-    }
-
-    return 0;
-}
-
 static void simple_dict(void)
 {
     int i;
     struct {
         const char *encoded;
-        LiteralQObject decoded;
+        QLitObject decoded;
     } test_cases[] = {
         {
             .encoded = "{\"foo\": 42, \"bar\": \"hello world\"}",
-            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
+            .decoded = QLIT_QDICT(((QLitDictEntry[]){
                         { "foo", QLIT_QNUM(42) },
                         { "bar", QLIT_QSTR("hello world") },
                         { }
                     })),
         }, {
             .encoded = "{}",
-            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
+            .decoded = QLIT_QDICT(((QLitDictEntry[]){
                         { }
                     })),
         }, {
             .encoded = "{\"foo\": 43}",
-            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
+            .decoded = QLIT_QDICT(((QLitDictEntry[]){
                         { "foo", QLIT_QNUM(43) },
                         { }
                     })),
@@ -1188,13 +1094,13 @@ static void simple_dict(void)
         QString *str;
 
         obj = qobject_from_json(test_cases[i].encoded, &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
         obj = qobject_from_json(qstring_get_str(str), &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
         qobject_decref(obj);
         QDECREF(str);
     }
@@ -1257,11 +1163,11 @@ static void simple_list(void)
     int i;
     struct {
         const char *encoded;
-        LiteralQObject decoded;
+        QLitObject decoded;
     } test_cases[] = {
         {
             .encoded = "[43,42]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(43),
                         QLIT_QNUM(42),
                         { }
@@ -1269,21 +1175,21 @@ static void simple_list(void)
         },
         {
             .encoded = "[43]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(43),
                         { }
                     })),
         },
         {
             .encoded = "[]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         { }
                     })),
         },
         {
             .encoded = "[{}]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
-                        QLIT_QDICT(((LiteralQDictEntry[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
+                        QLIT_QDICT(((QLitDictEntry[]){
                                     {},
                                         })),
                         {},
@@ -1297,13 +1203,13 @@ static void simple_list(void)
         QString *str;
 
         obj = qobject_from_json(test_cases[i].encoded, &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
         obj = qobject_from_json(qstring_get_str(str), &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
         qobject_decref(obj);
         QDECREF(str);
     }
@@ -1314,11 +1220,11 @@ static void simple_whitespace(void)
     int i;
     struct {
         const char *encoded;
-        LiteralQObject decoded;
+        QLitObject decoded;
     } test_cases[] = {
         {
             .encoded = " [ 43 , 42 ]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(43),
                         QLIT_QNUM(42),
                         { }
@@ -1326,12 +1232,12 @@ static void simple_whitespace(void)
         },
         {
             .encoded = " [ 43 , { 'h' : 'b' }, [ ], 42 ]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(43),
-                        QLIT_QDICT(((LiteralQDictEntry[]){
+                        QLIT_QDICT(((QLitDictEntry[]){
                                     { "h", QLIT_QSTR("b") },
                                     { }})),
-                        QLIT_QLIST(((LiteralQObject[]){
+                        QLIT_QLIST(((QLitObject[]){
                                     { }})),
                         QLIT_QNUM(42),
                         { }
@@ -1339,13 +1245,13 @@ static void simple_whitespace(void)
         },
         {
             .encoded = " [ 43 , { 'h' : 'b' , 'a' : 32 }, [ ], 42 ]",
-            .decoded = QLIT_QLIST(((LiteralQObject[]){
+            .decoded = QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(43),
-                        QLIT_QDICT(((LiteralQDictEntry[]){
+                        QLIT_QDICT(((QLitDictEntry[]){
                                     { "h", QLIT_QSTR("b") },
                                     { "a", QLIT_QNUM(32) },
                                     { }})),
-                        QLIT_QLIST(((LiteralQObject[]){
+                        QLIT_QLIST(((QLitObject[]){
                                     { }})),
                         QLIT_QNUM(42),
                         { }
@@ -1359,13 +1265,13 @@ static void simple_whitespace(void)
         QString *str;
 
         obj = qobject_from_json(test_cases[i].encoded, &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
         obj = qobject_from_json(qstring_get_str(str), &error_abort);
-        g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+        g_assert(qlit_equal_qobject(&test_cases[i].decoded, obj));
 
         qobject_decref(obj);
         QDECREF(str);
@@ -1376,10 +1282,10 @@ static void simple_varargs(void)
 {
     QObject *embedded_obj;
     QObject *obj;
-    LiteralQObject decoded = QLIT_QLIST(((LiteralQObject[]){
+    QLitObject decoded = QLIT_QLIST(((QLitObject[]){
             QLIT_QNUM(1),
             QLIT_QNUM(2),
-            QLIT_QLIST(((LiteralQObject[]){
+            QLIT_QLIST(((QLitObject[]){
                         QLIT_QNUM(32),
                         QLIT_QNUM(42),
                         {}})),
@@ -1389,7 +1295,7 @@ static void simple_varargs(void)
     g_assert(embedded_obj != NULL);
 
     obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
-    g_assert(compare_litqobj_to_qobj(&decoded, obj) == 1);
+    g_assert(qlit_equal_qobject(&decoded, obj));
 
     qobject_decref(obj);
 }
diff --git a/tests/check-qlit.c b/tests/check-qlit.c
new file mode 100644
index 0000000000..5736b61ebf
--- /dev/null
+++ b/tests/check-qlit.c
@@ -0,0 +1,68 @@
+/*
+ * QLit unit-tests.
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlit.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
+
+static QLitObject qlit = QLIT_QDICT(((QLitDictEntry[]) {
+    { "foo", QLIT_QNUM(42) },
+    { "bar", QLIT_QSTR("hello world") },
+    { "baz", QLIT_QNULL },
+    { "bee", QLIT_QLIST(((QLitObject[]) {
+        QLIT_QNUM(43),
+        QLIT_QNUM(44),
+        QLIT_QBOOL(true),
+        { },
+    }))},
+    { },
+}));
+
+static QObject *make_qobject(void)
+{
+    QDict *qdict = qdict_new();
+    QList *list = qlist_new();
+
+    qdict_put_int(qdict, "foo", 42);
+    qdict_put_str(qdict, "bar", "hello world");
+    qdict_put_null(qdict, "baz");
+
+    qlist_append_int(list, 43);
+    qlist_append_int(list, 44);
+    qlist_append_bool(list, true);
+    qdict_put(qdict, "bee", list);
+
+    return QOBJECT(qdict);
+}
+
+static void qlit_equal_qobject_test(void)
+{
+    QObject *qobj = make_qobject();
+    QList *list = qlist_new();
+
+    g_assert(qlit_equal_qobject(&qlit, qobj));
+
+    qdict_put(qobject_to_qdict(qobj), "bee", list);
+    g_assert(!qlit_equal_qobject(&qlit, qobj));
+
+    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);
+
+    return g_test_run();
+}
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index fc8885c9a4..002d25873a 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,2 +1,2 @@
-util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o
+util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o qlit.o
 util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 37c1bed683..3653c7b40a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -43,6 +43,8 @@ check-unit-y += tests/check-qnull$(EXESUF)
 gcov-files-check-qnull-y = qobject/qnull.c
 check-unit-y += tests/check-qjson$(EXESUF)
 gcov-files-check-qjson-y = qobject/qjson.c
+check-unit-y += tests/check-qlit$(EXESUF)
+gcov-files-check-qlit-y = qobject/qlit.c
 check-unit-y += tests/test-qobject-output-visitor$(EXESUF)
 gcov-files-test-qobject-output-visitor-y = qapi/qobject-output-visitor.c
 check-unit-y += tests/test-clone-visitor$(EXESUF)
@@ -535,7 +537,7 @@ GENERATED_FILES += tests/test-qapi-types.h tests/test-qapi-visit.h \
 
 test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
 	tests/check-qlist.o tests/check-qnull.o \
-	tests/check-qjson.o \
+	tests/check-qjson.o tests/check-qlit.o \
 	tests/test-coroutine.o tests/test-string-output-visitor.o \
 	tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
 	tests/test-clone-visitor.o \
@@ -569,6 +571,7 @@ tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
+tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
 tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
 
-- 
2.14.1.146.gd35faa819

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
> allowing to statically declare complex qobjects.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Your patch does more than that!  It also

* renames the now externally visible identifiers,

* adds support for qnull and qnum,

* cleans up types (int vs. bool) and style,

* makes compare_litqobj_to_qobj() case QTYPE_QNULL and QTYPE_QSTRING
  more robust, and

* fixes bugs in compare_litqobj_to_qobj() case QTYPE_QDICT, QTYPE_QLIST
  (I think).

Squashing the renames into the code motion is tolerable, but the rest
isn't, because it makes patch review harder for no benefit at all.
Moreover, the commit message fails to record the changes.  I noticed
them only because out of an abundance of caution I checked the patch is
just what it's advertised to be: code motion.  Well, it isn't.

Separate patches, please!

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
> > allowing to statically declare complex qobjects.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Your patch does more than that!  It also
> 
> * renames the now externally visible identifiers,
> 
> * adds support for qnull and qnum,
> 
> * cleans up types (int vs. bool) and style,
> 
> * makes compare_litqobj_to_qobj() case QTYPE_QNULL and QTYPE_QSTRING
>   more robust, and
> 
> * fixes bugs in compare_litqobj_to_qobj() case QTYPE_QDICT, QTYPE_QLIST
>   (I think).
> 
> Squashing the renames into the code motion is tolerable, but the rest
> isn't, because it makes patch review harder for no benefit at all.

The title said "add" and in the commit message "promote", it's not just a "move".

Imho, it's best to take a fresh look at the implementation since it is no longer in tests and can be used from qemu/programs.

As such you can review the code as "new" code, and check the corresponding tests. Finally, the existing test is simply adapted to that code.

> Moreover, the commit message fails to record the changes.  I noticed
> them only because out of an abundance of caution I checked the patch is
> just what it's advertised to be: code motion.  Well, it isn't.
> 
> Separate patches, please!

Sure, I can do that, I just thought it was extra overhead given my approach.

thanks

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> ----- Original Message -----
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> 
>> > Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
>> > allowing to statically declare complex qobjects.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> Your patch does more than that!  It also
>> 
>> * renames the now externally visible identifiers,
>> 
>> * adds support for qnull and qnum,
>> 
>> * cleans up types (int vs. bool) and style,
>> 
>> * makes compare_litqobj_to_qobj() case QTYPE_QNULL and QTYPE_QSTRING
>>   more robust, and
>> 
>> * fixes bugs in compare_litqobj_to_qobj() case QTYPE_QDICT, QTYPE_QLIST
>>   (I think).
>> 
>> Squashing the renames into the code motion is tolerable, but the rest
>> isn't, because it makes patch review harder for no benefit at all.
>
> The title said "add" and in the commit message "promote", it's not just a "move".

Yes, that's what it says, and it fooled me just fine %-}

> Imho, it's best to take a fresh look at the implementation since it is no longer in tests and can be used from qemu/programs.
>
> As such you can review the code as "new" code, and check the corresponding tests. Finally, the existing test is simply adapted to that code.
>
>> Moreover, the commit message fails to record the changes.  I noticed
>> them only because out of an abundance of caution I checked the patch is
>> just what it's advertised to be: code motion.  Well, it isn't.
>> 
>> Separate patches, please!
>
> Sure, I can do that, I just thought it was extra overhead given my approach.

Thanks.