[PATCH 00/10] Fix floating-point -> text conversion precision

[PATCH 00/10] Fix floating-point -> text conversion precision

[Markus Armbruster]

This takes care of ancient FIXMEs, mostly because a future series will
rework the code, and I can't bring myself tiptoeing around these bugs.

Markus Armbruster (10):
  tests/check-qjson: Don't skip funny QNumber to JSON conversions
  tests/check-qjson: Examine QNum more thoroughly
  tests/check-qjson: Cover number 2^63
  tests/check-qjson: Replace redundant large_number()
  tests/check-qnum: Cover qnum_to_string() for "unround" argument
  qobject: Fix qnum_to_string() to use sufficient precision
  test-string-output-visitor: Cover "unround" number
  string-output-visitor: Fix to use sufficient precision
  test-visitor-serialization: Drop insufficient precision workaround
  test-visitor-serialization: Clean up test_primitives()

 qapi/string-output-visitor.c       |   2 +-
 qobject/qnum.c                     |  24 +----
 tests/check-qjson.c                | 146 +++++++++++++++--------------
 tests/check-qnum.c                 |   8 +-
 tests/test-string-output-visitor.c |   4 +-
 tests/test-visitor-serialization.c |  62 +++++++-----
 6 files changed, 127 insertions(+), 119 deletions(-)

-- 
2.26.2

[PATCH 01/10] tests/check-qjson: Don't skip funny QNumber to JSON conversions

[Markus Armbruster]

simple_number() and float_number() convert from JSON to QNumber and
back.

simple_number() tests "-0", but skips the conversion back to JSON,
because it yields "0", not "-0".  Works as intended, so better cover
it: don't skip, but expect the funny result.

float_number() tests "-32.20e-10", but skips the conversion back to
JSON, because it yields "-0".  This is a known bug in
qnum_to_string(), marked FIXME there.  Cover the bug: don't skip, but
expect the funny result.

While there, switch from g_assert() to g_assert_cmpstr() & friends for
friendlier test failures.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 55 +++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 9a02079099..2a5852904a 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -793,37 +793,35 @@ static void utf8_string(void)
 
 static void simple_number(void)
 {
-    int i;
     struct {
         const char *encoded;
         int64_t decoded;
-        int skip;
+        const char *reencoded;
     } test_cases[] = {
         { "0", 0 },
         { "1234", 1234 },
         { "1", 1 },
         { "-32", -32 },
-        { "-0", 0, .skip = 1 },
-        { },
+        { "-0", 0, "0" },
+        {},
     };
+    int i;
+    QNum *qnum;
+    int64_t val;
+    QString *str;
 
     for (i = 0; test_cases[i].encoded; i++) {
-        QNum *qnum;
-        int64_t val;
-
         qnum = qobject_to(QNum,
                           qobject_from_json(test_cases[i].encoded,
                                             &error_abort));
         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;
 
-            str = qobject_to_json(QOBJECT(qnum));
-            g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
-            qobject_unref(str);
-        }
+        str = qobject_to_json(QOBJECT(qnum));
+        g_assert_cmpstr(qstring_get_str(str), ==,
+                        test_cases[i].reencoded ?: test_cases[i].encoded);
+        qobject_unref(str);
 
         qobject_unref(qnum);
     }
@@ -874,35 +872,32 @@ static void large_number(void)
 
 static void float_number(void)
 {
-    int i;
     struct {
         const char *encoded;
         double decoded;
-        int skip;
+        const char *reencoded;
     } test_cases[] = {
         { "32.43", 32.43 },
         { "0.222", 0.222 },
         { "-32.12313", -32.12313 },
-        { "-32.20e-10", -32.20e-10, .skip = 1 },
-        { },
+        { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },
+        {},
     };
+    int i;
+    QNum *qnum;
+    QString *str;
 
     for (i = 0; test_cases[i].encoded; i++) {
-        QObject *obj;
-        QNum *qnum;
-
-        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
-        qnum = qobject_to(QNum, obj);
+        qnum = qobject_to(QNum,
+                          qobject_from_json(test_cases[i].encoded,
+                                            &error_abort));
         g_assert(qnum);
-        g_assert(qnum_get_double(qnum) == test_cases[i].decoded);
+        g_assert_cmpfloat(qnum_get_double(qnum), ==, test_cases[i].decoded);
 
-        if (test_cases[i].skip == 0) {
-            QString *str;
-
-            str = qobject_to_json(obj);
-            g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
-            qobject_unref(str);
-        }
+        str = qobject_to_json(QOBJECT(qnum));
+        g_assert_cmpstr(qstring_get_str(str), ==,
+                        test_cases[i].reencoded ?: test_cases[i].encoded);
+        qobject_unref(str);
 
         qobject_unref(qnum);
     }
-- 
2.26.2

[PATCH 02/10] tests/check-qjson: Examine QNum more thoroughly

[Markus Armbruster]

simple_number() checks only qnum_get_try_int().  Also check
qnum_get_try_uint() and qnum_get_double().

float_number() checks only qnum_get_double().  Also check
qnum_get_try_int() and qnum_get_try_uint().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 2a5852904a..6ab6b111b0 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -807,7 +807,8 @@ static void simple_number(void)
     };
     int i;
     QNum *qnum;
-    int64_t val;
+    int64_t ival;
+    uint64_t uval;
     QString *str;
 
     for (i = 0; test_cases[i].encoded; i++) {
@@ -815,8 +816,16 @@ static void simple_number(void)
                           qobject_from_json(test_cases[i].encoded,
                                             &error_abort));
         g_assert(qnum);
-        g_assert(qnum_get_try_int(qnum, &val));
-        g_assert_cmpint(val, ==, test_cases[i].decoded);
+        g_assert(qnum_get_try_int(qnum, &ival));
+        g_assert_cmpint(ival, ==, test_cases[i].decoded);
+        if (test_cases[i].decoded >= 0) {
+            g_assert(qnum_get_try_uint(qnum, &uval));
+            g_assert_cmpuint(uval, ==, (uint64_t)test_cases[i].decoded);
+        } else {
+            g_assert(!qnum_get_try_uint(qnum, &uval));
+        }
+        g_assert_cmpfloat(qnum_get_double(qnum), ==,
+                          (double)test_cases[i].decoded);
 
         str = qobject_to_json(QOBJECT(qnum));
         g_assert_cmpstr(qstring_get_str(str), ==,
@@ -885,6 +894,8 @@ static void float_number(void)
     };
     int i;
     QNum *qnum;
+    int64_t ival;
+    uint64_t uval;
     QString *str;
 
     for (i = 0; test_cases[i].encoded; i++) {
@@ -893,6 +904,8 @@ static void float_number(void)
                                             &error_abort));
         g_assert(qnum);
         g_assert_cmpfloat(qnum_get_double(qnum), ==, test_cases[i].decoded);
+        g_assert(!qnum_get_try_int(qnum, &ival));
+        g_assert(!qnum_get_try_uint(qnum, &uval));
 
         str = qobject_to_json(QOBJECT(qnum));
         g_assert_cmpstr(qstring_get_str(str), ==,
-- 
2.26.2

[PATCH 03/10] tests/check-qjson: Cover number 2^63

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 6ab6b111b0..8cb8a40524 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -791,7 +791,7 @@ static void utf8_string(void)
     }
 }
 
-static void simple_number(void)
+static void int_number(void)
 {
     struct {
         const char *encoded;
@@ -836,6 +836,42 @@ static void simple_number(void)
     }
 }
 
+static void uint_number(void)
+{
+    struct {
+        const char *encoded;
+        uint64_t decoded;
+        const char *reencoded;
+    } test_cases[] = {
+        { "9223372036854775808", (uint64_t)1 << 63 },
+        {},
+    };
+    int i;
+    QNum *qnum;
+    int64_t ival;
+    uint64_t uval;
+    QString *str;
+
+    for (i = 0; test_cases[i].encoded; i++) {
+        qnum = qobject_to(QNum,
+                          qobject_from_json(test_cases[i].encoded,
+                                            &error_abort));
+        g_assert(qnum);
+        g_assert(qnum_get_try_uint(qnum, &uval));
+        g_assert_cmpuint(uval, ==, test_cases[i].decoded);
+        g_assert(!qnum_get_try_int(qnum, &ival));
+        g_assert_cmpfloat(qnum_get_double(qnum), ==,
+                          (double)test_cases[i].decoded);
+
+        str = qobject_to_json(QOBJECT(qnum));
+        g_assert_cmpstr(qstring_get_str(str), ==,
+                        test_cases[i].reencoded ?: test_cases[i].encoded);
+        qobject_unref(str);
+
+        qobject_unref(qnum);
+    }
+}
+
 static void large_number(void)
 {
     const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
@@ -1487,7 +1523,8 @@ int main(int argc, char **argv)
     g_test_add_func("/literals/string/quotes", string_with_quotes);
     g_test_add_func("/literals/string/utf8", utf8_string);
 
-    g_test_add_func("/literals/number/simple", simple_number);
+    g_test_add_func("/literals/number/int", int_number);
+    g_test_add_func("/literals/number/uint", uint_number);
     g_test_add_func("/literals/number/large", large_number);
     g_test_add_func("/literals/number/float", float_number);
 
-- 
2.26.2

[PATCH 04/10] tests/check-qjson: Replace redundant large_number()

[Markus Armbruster]

Move one of large_number()'s three checks to uint_number(), and the
other two to float_number().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 47 +++------------------------------------------
 1 file changed, 3 insertions(+), 44 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 8cb8a40524..98515b1fd6 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -844,6 +844,7 @@ static void uint_number(void)
         const char *reencoded;
     } test_cases[] = {
         { "9223372036854775808", (uint64_t)1 << 63 },
+        { "18446744073709551615", UINT64_MAX },
         {},
     };
     int i;
@@ -872,49 +873,6 @@ static void uint_number(void)
     }
 }
 
-static void large_number(void)
-{
-    const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
-    const char *gtu64 = "18446744073709551616"; /* 2^64 */
-    const char *lti64 = "-9223372036854775809"; /* -2^63 - 1 */
-    QNum *qnum;
-    QString *str;
-    uint64_t val;
-    int64_t ival;
-
-    qnum = qobject_to(QNum, qobject_from_json(maxu64, &error_abort));
-    g_assert(qnum);
-    g_assert_cmpuint(qnum_get_uint(qnum), ==, 18446744073709551615U);
-    g_assert(!qnum_get_try_int(qnum, &ival));
-
-    str = qobject_to_json(QOBJECT(qnum));
-    g_assert_cmpstr(qstring_get_str(str), ==, maxu64);
-    qobject_unref(str);
-    qobject_unref(qnum);
-
-    qnum = qobject_to(QNum, qobject_from_json(gtu64, &error_abort));
-    g_assert(qnum);
-    g_assert_cmpfloat(qnum_get_double(qnum), ==, 18446744073709552e3);
-    g_assert(!qnum_get_try_uint(qnum, &val));
-    g_assert(!qnum_get_try_int(qnum, &ival));
-
-    str = qobject_to_json(QOBJECT(qnum));
-    g_assert_cmpstr(qstring_get_str(str), ==, gtu64);
-    qobject_unref(str);
-    qobject_unref(qnum);
-
-    qnum = qobject_to(QNum, qobject_from_json(lti64, &error_abort));
-    g_assert(qnum);
-    g_assert_cmpfloat(qnum_get_double(qnum), ==, -92233720368547758e2);
-    g_assert(!qnum_get_try_uint(qnum, &val));
-    g_assert(!qnum_get_try_int(qnum, &ival));
-
-    str = qobject_to_json(QOBJECT(qnum));
-    g_assert_cmpstr(qstring_get_str(str), ==, "-9223372036854775808");
-    qobject_unref(str);
-    qobject_unref(qnum);
-}
-
 static void float_number(void)
 {
     struct {
@@ -926,6 +884,8 @@ static void float_number(void)
         { "0.222", 0.222 },
         { "-32.12313", -32.12313 },
         { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },
+        { "18446744073709551616", 0x1p64 },
+        { "-9223372036854775809", -0x1p63, "-9223372036854775808" },
         {},
     };
     int i;
@@ -1525,7 +1485,6 @@ int main(int argc, char **argv)
 
     g_test_add_func("/literals/number/int", int_number);
     g_test_add_func("/literals/number/uint", uint_number);
-    g_test_add_func("/literals/number/large", large_number);
     g_test_add_func("/literals/number/float", float_number);
 
     g_test_add_func("/literals/keyword", keyword_literal);
-- 
2.26.2

[PATCH 05/10] tests/check-qnum: Cover qnum_to_string() for "unround" argument

[Markus Armbruster]

qnum_to_string() has a FIXME comment about rounding errors due to
insufficient precision.  Cover it: 2.718281828459045 gets converted to
"2.718282".  The next commit will fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qnum.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/check-qnum.c b/tests/check-qnum.c
index 4105015872..a73809d021 100644
--- a/tests/check-qnum.c
+++ b/tests/check-qnum.c
@@ -150,6 +150,12 @@ static void qnum_to_string_test(void)
     g_assert_cmpstr(tmp, ==, "0.42");
     g_free(tmp);
     qobject_unref(qn);
+
+    qn = qnum_from_double(2.718281828459045);
+    tmp = qnum_to_string(qn);
+    g_assert_cmpstr(tmp, ==, "2.718282"); /* BUG */
+    g_free(tmp);
+    qobject_unref(qn);
 }
 
 int main(int argc, char **argv)
-- 
2.26.2

[PATCH 06/10] qobject: Fix qnum_to_string() to use sufficient precision

[Markus Armbruster]

We should serialize numbers to JSON so that they deserialize back to
the same number.  We fail to do so.

The culprit is qnum_to_string(): it uses format %f with trailing '0'
trimmed.  Results in pretty output for "nice" numbers, but is prone to
nasty rounding errors.  For instance, numbers between 0 and 0.0000005
get flushed to zero.

Where exactly the incorrect rounding can bite is tiresome to gauge.
Here's my take.

* In QMP output, type 'number':

  - query-blockstats value avg_rd_queue_depth

  - QMP query-migrate values mbps, cache-miss-rate, encoding-rate,
    busy-rate, compression-rate.

  Relatively harmless, I guess.

* In tracing QMP input.  Harmless.

* In qemu-ga output, type 'number': guest-get-users value login-time.
  Harmless.

* In output of HMP qom-get.  Harmless.

Not affected, because double values don't actually occur there (I
think):

* QMP output, type 'any':

  * qom-get value

  * qom-list, qom-list-properties value default-value

  * query-cpu-model-comparison, query-cpu-model-baseline,
    query-cpu-model-expansion value props.

* qemu-img --output json output.

* "json:" pseudo-filenames generated by bdrv_refresh_filename().

* The rbd block driver's "=keyvalue-pairs" hack.

* In -object help on property default values.  Aside: use of JSON
  feels inappropriate here.

* Output of HMP qom-get.

* Argument conversion to QemuOpts for qdev_device_add() and HMP with
  qemu_opts_from_qdict()

  QMP and HMP device_add, virtio-net failover primary creation,
  xen-usb "usb-host" creation, HMP netdev_add, object_add.

* The uses of qobject_input_visitor_new_flat_confused()

  As far as I can tell, none of the visited types contain double
  values.

* Dumping ImageInfoSpecific with dump_qobject()

Fix by formatting with %.17g.  17 decimal digits always suffice for
IEEE double.

The change to expected test output illustrates the effect: the
rounding errors are gone, but some seemingly "nice" numbers now get
converted to not so nice strings, e.g. 0.42 to "0.41999999999999998".
This is because 0.42 is not representable exactly in double.  It's
more accurate in this example than strictly necessary, though.

If ugly accuracy bothers us, we can we can try using the least number
of digits that still converts back to the same double.  In this
example, "0.42" would do.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qnum.c      | 24 +++---------------------
 tests/check-qjson.c |  8 ++++----
 tests/check-qnum.c  |  4 ++--
 3 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/qobject/qnum.c b/qobject/qnum.c
index 7012fc57f2..bf1240ecec 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -161,37 +161,19 @@ double qnum_get_double(QNum *qn)
 
 char *qnum_to_string(QNum *qn)
 {
-    char *buffer;
-    int len;
-
     switch (qn->kind) {
     case QNUM_I64:
         return g_strdup_printf("%" PRId64, qn->u.i64);
     case QNUM_U64:
         return g_strdup_printf("%" PRIu64, qn->u.u64);
     case QNUM_DOUBLE:
-        /* FIXME: snprintf() is locale dependent; but JSON requires
+        /* FIXME: g_strdup_printf() is locale dependent; but JSON requires
          * numbers to be formatted as if in the C locale. Dependence
          * on C locale is a pervasive issue in QEMU. */
         /* FIXME: This risks printing Inf or NaN, which are not valid
          * JSON values. */
-        /* FIXME: the default precision of 6 for %f often causes
-         * rounding errors; we should be using DBL_DECIMAL_DIG (17),
-         * and only rounding to a shorter number if the result would
-         * still produce the same floating point value.  */
-        buffer = g_strdup_printf("%f" , qn->u.dbl);
-        len = strlen(buffer);
-        while (len > 0 && buffer[len - 1] == '0') {
-            len--;
-        }
-
-        if (len && buffer[len - 1] == '.') {
-            buffer[len - 1] = 0;
-        } else {
-            buffer[len] = 0;
-        }
-
-        return buffer;
+        /* 17 digits suffice for IEEE double */
+        return g_strdup_printf("%.17g", qn->u.dbl);
     }
 
     assert(0);
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 98515b1fd6..ca8fb816e9 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -882,10 +882,10 @@ static void float_number(void)
     } test_cases[] = {
         { "32.43", 32.43 },
         { "0.222", 0.222 },
-        { "-32.12313", -32.12313 },
-        { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },
-        { "18446744073709551616", 0x1p64 },
-        { "-9223372036854775809", -0x1p63, "-9223372036854775808" },
+        { "-32.12313", -32.12313, "-32.123130000000003" },
+        { "-32.20e-10", -32.20e-10, "-3.22e-09" },
+        { "18446744073709551616", 0x1p64, "1.8446744073709552e+19" },
+        { "-9223372036854775809", -0x1p63, "-9.2233720368547758e+18" },
         {},
     };
     int i;
diff --git a/tests/check-qnum.c b/tests/check-qnum.c
index a73809d021..b85fca2302 100644
--- a/tests/check-qnum.c
+++ b/tests/check-qnum.c
@@ -147,13 +147,13 @@ static void qnum_to_string_test(void)
 
     qn = qnum_from_double(0.42);
     tmp = qnum_to_string(qn);
-    g_assert_cmpstr(tmp, ==, "0.42");
+    g_assert_cmpstr(tmp, ==, "0.41999999999999998");
     g_free(tmp);
     qobject_unref(qn);
 
     qn = qnum_from_double(2.718281828459045);
     tmp = qnum_to_string(qn);
-    g_assert_cmpstr(tmp, ==, "2.718282"); /* BUG */
+    g_assert_cmpstr(tmp, ==, "2.7182818284590451");
     g_free(tmp);
     qobject_unref(qn);
 }
-- 
2.26.2

[PATCH 07/10] test-string-output-visitor: Cover "unround" number

[Markus Armbruster]

This demonstrates rounding error due to insufficient precision: double
3.1415926535897932 gets converted to JSON 3.141593.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-string-output-visitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 9f6581439a..cec20848ea 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -130,13 +130,13 @@ static void test_visitor_out_bool(TestOutputVisitorData *data,
 static void test_visitor_out_number(TestOutputVisitorData *data,
                                     const void *unused)
 {
-    double value = 3.14;
+    double value = 3.1415926535897932;
     char *str;
 
     visit_type_number(data->ov, NULL, &value, &error_abort);
 
     str = visitor_get(data);
-    g_assert_cmpstr(str, ==, "3.140000");
+    g_assert_cmpstr(str, ==, "3.141593");
 }
 
 static void test_visitor_out_string(TestOutputVisitorData *data,
-- 
2.26.2

[PATCH 08/10] string-output-visitor: Fix to use sufficient precision

[Markus Armbruster]

The string output visitor should serialize numbers so that the string
input visitor deserializes them back to the same number.  It fails to
do so.

print_type_number() uses format %f.  This is prone to nasty rounding
errors.  For instance, numbers between 0 and 0.0000005 get flushed to
zero.

We currently use this visitor only for HMP info migrate, info network,
info qtree, and info memdev.  No double values occur there as far as I
can tell.

Fix anyway by formatting with %.17g.  17 decimal digits always suffice
for IEEE double.

See also recent commit "qobject: Fix qnum_to_string() to use
sufficient precision".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/string-output-visitor.c       | 2 +-
 tests/test-string-output-visitor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b74aa4d44c..5506c933de 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -258,7 +258,7 @@ static bool print_type_number(Visitor *v, const char *name, double *obj,
                               Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);
-    string_output_set(sov, g_strdup_printf("%f", *obj));
+    string_output_set(sov, g_strdup_printf("%.17g", *obj));
     return true;
 }
 
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index cec20848ea..0dae04b960 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -136,7 +136,7 @@ static void test_visitor_out_number(TestOutputVisitorData *data,
     visit_type_number(data->ov, NULL, &value, &error_abort);
 
     str = visitor_get(data);
-    g_assert_cmpstr(str, ==, "3.141593");
+    g_assert_cmpstr(str, ==, "3.1415926535897931");
 }
 
 static void test_visitor_out_string(TestOutputVisitorData *data,
-- 
2.26.2

[PATCH 09/10] test-visitor-serialization: Drop insufficient precision workaround

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-visitor-serialization.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 1c5a8b94ea..19b72571bb 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -311,17 +311,7 @@ static void test_primitives(gconstpointer opaque)
         g_assert_cmpstr(pt->value.string, ==, pt_copy->value.string);
         g_free((char *)pt_copy->value.string);
     } else if (pt->type == PTYPE_NUMBER) {
-        GString *double_expected = g_string_new("");
-        GString *double_actual = g_string_new("");
-        /* we serialize with %f for our reference visitors, so rather than fuzzy
-         * floating math to test "equality", just compare the formatted values
-         */
-        g_string_printf(double_expected, "%.6f", pt->value.number);
-        g_string_printf(double_actual, "%.6f", pt_copy->value.number);
-        g_assert_cmpstr(double_actual->str, ==, double_expected->str);
-
-        g_string_free(double_expected, true);
-        g_string_free(double_actual, true);
+        g_assert_cmpfloat(pt->value.number, ==, pt_copy->value.number);
     } else if (pt->type == PTYPE_BOOLEAN) {
         g_assert_cmpint(!!pt->value.max, ==, !!pt->value.max);
     } else {
@@ -790,10 +780,6 @@ static PrimitiveType pt_values[] = {
         .value.boolean = 0,
     },
     /* number tests (double) */
-    /* note: we format these to %.6f before comparing, since that's how
-     * we serialize them and it doesn't make sense to check precision
-     * beyond that.
-     */
     {
         .description = "number_sanity1",
         .type = PTYPE_NUMBER,
@@ -802,7 +788,7 @@ static PrimitiveType pt_values[] = {
     {
         .description = "number_sanity2",
         .type = PTYPE_NUMBER,
-        .value.number = 3.14159265,
+        .value.number = 3.141593,
     },
     {
         .description = "number_min",
-- 
2.26.2

[PATCH 10/10] test-visitor-serialization: Clean up test_primitives()

[Markus Armbruster]

test_primitives() uses union member intmax_t max to compare the
integer members.  Unspecified behavior.  Has worked fine for many
years, though.  Clean it up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-visitor-serialization.c | 44 +++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 19b72571bb..7eab18b7c6 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -55,7 +55,6 @@ typedef struct PrimitiveType {
         int16_t s16;
         int32_t s32;
         int64_t s64;
-        intmax_t max;
     } value;
     enum PrimitiveTypeKind type;
     const char *description;
@@ -307,15 +306,46 @@ static void test_primitives(gconstpointer opaque)
                      &error_abort);
 
     g_assert(pt_copy != NULL);
-    if (pt->type == PTYPE_STRING) {
+    switch (pt->type) {
+    case PTYPE_STRING:
         g_assert_cmpstr(pt->value.string, ==, pt_copy->value.string);
         g_free((char *)pt_copy->value.string);
-    } else if (pt->type == PTYPE_NUMBER) {
+        break;
+    case PTYPE_BOOLEAN:
+        g_assert_cmpint(pt->value.boolean, ==, pt->value.boolean);
+        break;
+    case PTYPE_NUMBER:
         g_assert_cmpfloat(pt->value.number, ==, pt_copy->value.number);
-    } else if (pt->type == PTYPE_BOOLEAN) {
-        g_assert_cmpint(!!pt->value.max, ==, !!pt->value.max);
-    } else {
-        g_assert_cmpint(pt->value.max, ==, pt_copy->value.max);
+        break;
+    case PTYPE_INTEGER:
+        g_assert_cmpint(pt->value.integer, ==, pt_copy->value.integer);
+        break;
+    case PTYPE_U8:
+        g_assert_cmpuint(pt->value.u8, ==, pt_copy->value.u8);
+        break;
+    case PTYPE_U16:
+        g_assert_cmpuint(pt->value.u16, ==, pt_copy->value.u16);
+        break;
+    case PTYPE_U32:
+        g_assert_cmpuint(pt->value.u32, ==, pt_copy->value.u32);
+        break;
+    case PTYPE_U64:
+        g_assert_cmpuint(pt->value.u64, ==, pt_copy->value.u64);
+        break;
+    case PTYPE_S8:
+        g_assert_cmpint(pt->value.s8, ==, pt_copy->value.s8);
+        break;
+    case PTYPE_S16:
+        g_assert_cmpint(pt->value.s16, ==, pt_copy->value.s16);
+        break;
+    case PTYPE_S32:
+        g_assert_cmpint(pt->value.s32, ==, pt_copy->value.s32);
+        break;
+    case PTYPE_S64:
+        g_assert_cmpint(pt->value.s64, ==, pt_copy->value.s64);
+        break;
+    case PTYPE_EOL:
+        g_assert_not_reached();
     }
 
     ops->cleanup(serialize_data);
-- 
2.26.2

Re: [PATCH 00/10] Fix floating-point -> text conversion precision

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201210161452.2813491-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201210161452.2813491-1-armbru@redhat.com
Subject: [PATCH 00/10] Fix floating-point -> text conversion precision

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200916095150.755714-1-stefanha@redhat.com -> patchew/20200916095150.755714-1-stefanha@redhat.com
 * [new tag]         patchew/20201210161452.2813491-1-armbru@redhat.com -> patchew/20201210161452.2813491-1-armbru@redhat.com
Switched to a new branch 'test'
b484d41 test-visitor-serialization: Clean up test_primitives()
2284fd5 test-visitor-serialization: Drop insufficient precision workaround
be2e608 string-output-visitor: Fix to use sufficient precision
efd2a57 test-string-output-visitor: Cover "unround" number
9ed1edd qobject: Fix qnum_to_string() to use sufficient precision
0cdcb97 tests/check-qnum: Cover qnum_to_string() for "unround" argument
a349bc3 tests/check-qjson: Replace redundant large_number()
00db712 tests/check-qjson: Cover number 2^63
487c226 tests/check-qjson: Examine QNum more thoroughly
b783de6 tests/check-qjson: Don't skip funny QNumber to JSON conversions

=== OUTPUT BEGIN ===
1/10 Checking commit b783de656e9b (tests/check-qjson: Don't skip funny QNumber to JSON conversions)
WARNING: Block comments use a leading /* on a separate line
#94: FILE: tests/check-qjson.c:883:
+        { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },

ERROR: spaces required around that '-' (ctx:VxV)
#94: FILE: tests/check-qjson.c:883:
+        { "-32.20e-10", -32.20e-10, "-0" /* BUG */ },
                                ^

total: 1 errors, 1 warnings, 97 lines checked

Patch 1/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/10 Checking commit 487c226c8132 (tests/check-qjson: Examine QNum more thoroughly)
3/10 Checking commit 00db712e3d42 (tests/check-qjson: Cover number 2^63)
4/10 Checking commit a349bc3f2fb5 (tests/check-qjson: Replace redundant large_number())
5/10 Checking commit 0cdcb9730d90 (tests/check-qnum: Cover qnum_to_string() for "unround" argument)
6/10 Checking commit 9ed1edd2eb94 (qobject: Fix qnum_to_string() to use sufficient precision)
WARNING: Block comments use a leading /* on a separate line
#104: FILE: qobject/qnum.c:170:
+        /* FIXME: g_strdup_printf() is locale dependent; but JSON requires

ERROR: spaces required around that '-' (ctx:VxV)
#144: FILE: tests/check-qjson.c:886:
+        { "-32.20e-10", -32.20e-10, "-3.22e-09" },
                                ^

total: 1 errors, 1 warnings, 69 lines checked

Patch 6/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/10 Checking commit efd2a57e5d20 (test-string-output-visitor: Cover "unround" number)
8/10 Checking commit be2e6080e5de (string-output-visitor: Fix to use sufficient precision)
9/10 Checking commit 2284fd528b2c (test-visitor-serialization: Drop insufficient precision workaround)
10/10 Checking commit b484d4126865 (test-visitor-serialization: Clean up test_primitives())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201210161452.2813491-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com