Eric Blake <ebl...@redhat.com> writes: > On 07/31/2017 07:34 AM, Eric Blake wrote: >> On 07/31/2017 03:16 AM, Markus Armbruster wrote: >> >>>>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict? >>>>> >> >>>> So given the clean bill of health from valgrind, we definitely DO turn >>>> over responsibility for freeing on object to its new wrapper, once it is >>>> passed through %p. Documentation could be improved to make that clear. >>>> >>>> Eww, what happens if qobject_from_jsonf() can fail halfway through? > > Hmm. What if we assert that qobject_from_jsonf() can never fail halfway > through? Given my research on the other thread, gcc -Wformat will NOT > flag "['%s', %p]",str,obj as a mismatch, although our current code will > try to associate %p with str and probably die horribly when > dereferencing char* as QObject* (and if it does NOT die, we don't even > know that 'obj' was passed as a parameter). Since the primary usage of > qobject_from_jsonf() is the testsuite, an assertion failure (forcing all > clients to use the interface correctly) is probably simpler than even > trying to have to worry about cleanup after partial failure (regardless > of whether we like the 'none' or 'all' approach).
I don't buy the "primarily for the testsuite argument". It's for whatever finds it useful. Safer than formatting the JSON text (no JSON injection accidents), much easier to read than building a QObject to pass to the JSON output visitor. I'd be willing to buy a "passing syntactically incorrect JSON to qobject_from_jsonf() is a programming error" argument. Assertion failure is the appropriate way to deal with programming errors. Needs to be spelled out in function contracts, of course, starting with json_parser_parse_err(), which (scandalously!) has none.
