Re: [Qemu-devel] [PATCH v2 07/60] check-qjson: Cover escaped characters more thoroughly, part 1

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/17/2018 10:05 AM, Markus Armbruster wrote:
>> escaped_string() first tests double quoted strings, then repeats a few
>> tests with single quotes.  Repeat all of them: store the strings to
>> test without quotes, and wrap them in either kind of quote for
>> testing.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   tests/check-qjson.c | 96 +++++++++++++++++++++++++++------------------
>>   1 file changed, 57 insertions(+), 39 deletions(-)
>>
>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> index 188f683317..2f1890929d 100644
>> --- a/tests/check-qjson.c
>> +++ b/tests/check-qjson.c
>> @@ -22,55 +22,73 @@
>>   #include "qapi/qmp/qstring.h"
>>   #include "qemu-common.h"
>>   +static QString *from_json_str(const char *jstr, Error **errp,
>> bool single)
>
> Unusual to put Error **errp in the middle rather than last. Worth
> swapping 'single' to come before 'errp'?

Absolutely.

>>          int skip;
>
> Pre-existing, but since you are touching this, is it worth making this bool?

There's more of the same in other functions, which aren't touched by
this patch.  If we want to convert to bool, we should convert all of
them.  Doesn't fit into this patch.

>> +    for (i = 0; test_cases[i].json_in; i++) {
>> +        for (j = 0; j < 2; j++) {
>> +            cstr = from_json_str(test_cases[i].json_in, &error_abort, j);
>
> Only one caller to adjust here (and maybe a couple more later in the
> series).
>
> That's bike-shedding, so either way,
> Reviewed-by: Eric Blake <address@hidden>

Thanks!