Re: [Qemu-devel] [PATCH 1/2] qobject: Use 'bool' for qbool

["Andreas Färber" <afaerber@suse.de>]

Am 16.05.2015 um 00:24 schrieb Eric Blake:
> We require a C99 compiler, so let's use 'bool' instead of 'int'
> when dealing with boolean values.  There are few enough clients
> to fix them all in one pass.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qapi.c                    |  2 +-
>  block/quorum.c                  |  4 ++--
>  block/vvfat.c                   |  4 ++--
>  include/qapi/qmp/qbool.h        |  8 ++++----
>  monitor.c                       | 10 +++++-----
>  qapi/qmp-input-visitor.c        |  2 +-
>  qapi/qmp-output-visitor.c       |  2 +-
>  qobject/json-parser.c           |  6 +++---
>  qobject/qbool.c                 |  8 ++++----
>  qobject/qdict.c                 |  4 ++--
>  qobject/qjson.c                 |  2 +-
>  qom/object.c                    |  4 ++--
>  tests/check-qjson.c             | 11 ++++++-----
>  tests/test-qmp-event.c          |  4 ++--
>  tests/test-qmp-output-visitor.c |  4 ++--
>  util/qemu-option.c              |  2 +-
>  16 files changed, 39 insertions(+), 38 deletions(-)
[...]
> diff --git a/qobject/qbool.c b/qobject/qbool.c
> index a3d2afa..5ff69f0 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -23,11 +23,11 @@ static const QType qbool_type = {
>  };
> 
>  /**
> - * qbool_from_int(): Create a new QBool from an int
> + * qbool_from_bool(): Create a new QBool from a bool
>   *
>   * Return strong reference.

Can you fix the syntax as follow-up please?

/**
 * qbool_from_bool:
 * @value: ...
 *
 * Desc...
 *
 * Returns: ...
 */

>   */
> -QBool *qbool_from_int(int value)
> +QBool *qbool_from_bool(bool value)
>  {
>      QBool *qb;
> 
> @@ -39,9 +39,9 @@ QBool *qbool_from_int(int value)
>  }
> 
>  /**
> - * qbool_get_int(): Get the stored int
> + * qbool_get_bool(): Get the stored bool
>   */
> -int qbool_get_int(const QBool *qb)
> +bool qbool_get_bool(const QBool *qb)
>  {
>      return qb->value;
>  }
[...]
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index 60e5b22..1cfffa5 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -1013,7 +1013,7 @@ static void keyword_literal(void)
>      g_assert(qobject_type(obj) == QTYPE_QBOOL);
> 
>      qbool = qobject_to_qbool(obj);
> -    g_assert(qbool_get_int(qbool) != 0);
> +    g_assert(qbool_get_bool(qbool) == true);
> 
>      str = qobject_to_json(obj);
>      g_assert(strcmp(qstring_get_str(str), "true") == 0);
> @@ -1026,7 +1026,7 @@ static void keyword_literal(void)
>      g_assert(qobject_type(obj) == QTYPE_QBOOL);
> 
>      qbool = qobject_to_qbool(obj);
> -    g_assert(qbool_get_int(qbool) == 0);
> +    g_assert(qbool_get_bool(qbool) == false);
> 
>      str = qobject_to_json(obj);
>      g_assert(strcmp(qstring_get_str(str), "false") == 0);
> @@ -1039,16 +1039,17 @@ static void keyword_literal(void)
>      g_assert(qobject_type(obj) == QTYPE_QBOOL);
> 
>      qbool = qobject_to_qbool(obj);
> -    g_assert(qbool_get_int(qbool) == 0);
> +    g_assert(qbool_get_bool(qbool) == false);
> 
>      QDECREF(qbool);
> 
> -    obj = qobject_from_jsonf("%i", true);
> +    /* Test that non-zero values other than 1 get collapsed to true */
> +    obj = qobject_from_jsonf("%i", 2);
>      g_assert(obj != NULL);
>      g_assert(qobject_type(obj) == QTYPE_QBOOL);
> 
>      qbool = qobject_to_qbool(obj);
> -    g_assert(qbool_get_int(qbool) != 0);
> +    g_assert(qbool_get_bool(qbool) == true);
> 
>      QDECREF(qbool);
> 
[...]
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index f8c9367..5be8e77 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -72,7 +72,7 @@ static void test_visitor_out_bool(TestOutputVisitorData *data,
>      obj = qmp_output_get_qobject(data->qov);
>      g_assert(obj != NULL);
>      g_assert(qobject_type(obj) == QTYPE_QBOOL);
> -    g_assert(qbool_get_int(qobject_to_qbool(obj)) == value);
> +    g_assert(qbool_get_bool(qobject_to_qbool(obj)) == value);
> 
>      qobject_decref(obj);
>  }
> @@ -662,7 +662,7 @@ static void check_native_list(QObject *qobj,
>              tmp = qlist_peek(qlist);
>              g_assert(tmp);
>              qvalue = qobject_to_qbool(tmp);
> -            g_assert_cmpint(qbool_get_int(qvalue), ==, (i % 3 == 0) ? 1 : 0);
> +            g_assert_cmpint(qbool_get_bool(qvalue), ==, i % 3 == 0);
>              qobject_decref(qlist_pop(qlist));
>          }
>          break;
[snip]

I notice that we're inconsistent in using g_assert() vs.
g_assert_cmpint(). Given that GLib has a weird GBoolean, should we add a
macro qtest_assert_cmpbool() instead as follow-up?

That said,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)