On 06/11/2015 11:35 PM, Markus Armbruster wrote: > Patch looks good to me, but it made me wonder about something. Please > find the question inline. > > Eric Blake <[email protected]> writes: > >> 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. >>
>> @@ -593,7 +593,7 @@ static QObject *parse_escape(JSONParserContext *ctxt,
>> va_list *ap)
>> if (token_is_escape(token, "%p")) {
>> obj = va_arg(*ap, QObject *);
>> } else if (token_is_escape(token, "%i")) {
>> - obj = QOBJECT(qbool_from_int(va_arg(*ap, int)));
>> + obj = QOBJECT(qbool_from_bool(va_arg(*ap, int)));
>
> Funny: JSON_ESCAPE "%i" gets an int, but maps it to bool. See also
> patch to check-qjson.c below.
>
> Is this feature actually used anywhere other than the tests?
>
When using va_arg(), you have to use the type argument that things
promote to when called through var-args (that is, va_arg(*ap, bool)
would be a compiler warning).
I don't know if anyone besides the testsuite is using %i; but I've
already mentioned in another thread [1] that the correlation between...
>>
>> - 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);
>
> These are test test cases for JSON_ESCAPE "%i".
...qobject_from_json in one file to the implementation of %i in another
was very hard to trace when writing this patch, as well as the idea of
getting rid of the remaining 2 out of only 3 clients of %p [1]. So it's
already on my table of ideas to do a followup patch that adds
documentation, and audits all clients to see what can be pruned.
[1] https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04660.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
