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. > > 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature