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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 05/16/2015 07:30 AM, Andreas Färber wrote:
>> 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>
>>> ---
>
>>>  /**
>>> - * 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: ...
>>  */
>
> Sure, I can do that over all the qboject files, as a new patch.

Please don't, it's an egregious waste of screen space and the reader's
mental energy.

We're not using GTK-Doc for anything, and as long as we don't, I'm
unwilling to pay its price of admission.

If a maintainer prefers to enforce GTK-Doc comment syntax in his
subsystem, I won't argue.  In the (few) places I maintain, I'll insist
on readable, concise comments.  The @sigils are welcome, repeating
obvious things and other waste of space is not.  For what it's worth,
Kevin shared this sentiment last time we discussed it.

Luiz's call, because he's the maintainer.

>>> @@ -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?
>
> We aren't even touching GBoolean (qbool_get_bool now returns 'bool', not
> GBoolean; and bool promotes just fine to int under C rules), so I don't
> see the point to making any further changes here.  Or are you proposing
> that the new macro would do something like 'expecting "true" but got
> "false"' instead of g_assert_cmpint() collapsing things to 0 and 1?

Promoting to int here is just fine with me.

Andreas is right on GBoolean of course.  It's a relic that has become a
trap for the unwary.  Let's stay away from it as much as we can.