Eric Blake <ebl...@redhat.com> writes:
> On 11/04/2015 01:40 AM, Markus Armbruster wrote:
>
>>
>>> By moving err into data, we can let test teardown take care
>>> of cleaning up any collected error; it also gives us fewer
>>> lines of code between repeated tests where init runs teardown
>>> on our behalf.
>>
>> This part isn't as obvious.
>>
>> Having two parts of differing obviousness indicates patch splitting
>> could make sense. Especially when the parts are large and mechanical,
>> because reviewing large mechanical changes is much easier when there's
>> just one kind of it.
>
> Will split.
>
>>> @@ -364,21 +341,17 @@ static void
>>> test_visitor_in_alternate_number(TestInputVisitorData *data,
>>> /* Parsing an int */
>>>
>>> v = visitor_input_test_init(data, "42");
>>> - visit_type_AltStrBool(v, &asb, NULL, &err);
>>> - g_assert(err);
>>> - error_free(err);
>>> - err = NULL;
>>> + visit_type_AltStrBool(v, &asb, NULL, &data->err);
>>> + g_assert(data->err);
>>> qapi_free_AltStrBool(asb);
>>
>> This leaves data->err non-null.
>>
>>>
>>> /* FIXME: Order of alternate should not affect semantics; asn should
>>> * parse the same as ans */
>>> v = visitor_input_test_init(data, "42");
>
> And this part wipes out data, so that data->err is once again NULL.
>
>>> - visit_type_AltStrNum(v, &asn, NULL, &err);
>>> + visit_type_AltStrNum(v, &asn, NULL, &data->err);
>>
>> If visit_type_AltStrNum() errors out, its error will be thrown away by
>> its error_propagate(), because data->err is already non-null.
>
> No, you missed that this patch is relying on the magic in 3/27 that
> wipes out data on every new test.
You're right.
>> If it fails to error out, its error_propagate() does nothing, and we
>> won't detect the test failure.
>>
>> Your patch makes forgetting to reset err an even easier mistake to make,
>> because it removes most of the error_free() that serve as reminders.
>>
>> Is it a good idea anyway? Let's discuss before I check the remainder of
>> this patch.
>
> The point was that by moving err _into_ data, then the resetting of err
> becomes as simple as resetting data, rather than having to be verbose at
> every use of data->err. We just need a visitor_input_test_init() in
> between.
Reducing boilerplate is good. However, I'm afraid hiding
error_free(err); err = NULL; like this sets an example that is easily
copied incorrectly.
Here's what error.h has to say about consuming an Error object:
* Report an error to stderr:
* error_report_err(err);
* This frees the error object.
*
* Report an error somewhere else:
* const char *msg = error_get_pretty(err);
* do with msg what needs to be done...
* error_free(err);
*
* Handle an error without reporting it (just for completeness):
* error_free(err);
Perhaps we want something like
* Expect an error, abort() if there is none:
* error_free_or_abort(&err);
* This frees the error object and clears err. Convenient for tests.
Then the patch quoted above becomes
@@ -364,21 +341,17 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
/* Parsing an int */
v = visitor_input_test_init(data, "42");
visit_type_AltStrBool(v, &asb, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
/* FIXME: Order of alternate should not affect semantics; asn should
* parse the same as ans */
v = visitor_input_test_init(data, "42");
visit_type_AltStrNum(v, &asn, NULL, &err);
/* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
/* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrNum(asn);
v = visitor_input_test_init(data, "42");
Similar boilerplate reduction, but keeps the clearing of err more
visible.
