From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8jxu-0000oc-6z for qemu-devel@nongnu.org; Fri, 03 Jun 2016 03:56:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8jxr-0005KN-Mi for qemu-devel@nongnu.org; Fri, 03 Jun 2016 03:56:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50333) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8jxr-0005KH-Dm for qemu-devel@nongnu.org; Fri, 03 Jun 2016 03:56:19 -0400 From: Markus Armbruster References: <1463632874-28559-1-git-send-email-eblake@redhat.com> <1463632874-28559-26-git-send-email-eblake@redhat.com> Date: Fri, 03 Jun 2016 09:56:15 +0200 In-Reply-To: <1463632874-28559-26-git-send-email-eblake@redhat.com> (Eric Blake's message of "Wed, 18 May 2016 22:41:11 -0600") Message-ID: <87shwu66zk.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 25/28] qapi: Support pretty printing in JSON output visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > Similar to pretty printing in the QObject visitor. The trickiest > part is probably that the testsuite now has to honor parameterization > on whether pretty printing is enabled. Worth mentioning that the pretty-printing matches the one in qobject-json.c? > > Signed-off-by: Eric Blake > > --- > v4: rebase to earlier changes, defer type_any() to later > v3: rebase to earlier changes > v2: rebase to earlier changes > --- > include/qapi/json-output-visitor.h | 5 +- > qapi/json-output-visitor.c | 15 ++++- > tests/test-json-output-visitor.c | 122 +++++++++++++++++++++++++------------ > 3 files changed, 99 insertions(+), 43 deletions(-) > > diff --git a/include/qapi/json-output-visitor.h b/include/qapi/json-output-visitor.h > index 41c79f4..94c9e0f 100644 > --- a/include/qapi/json-output-visitor.h > +++ b/include/qapi/json-output-visitor.h > @@ -21,8 +21,11 @@ typedef struct JsonOutputVisitor JsonOutputVisitor; > * If everything else succeeds, pass @result to visit_complete() to > * collect the result of the visit. > * > + * If @pretty, make the output legible with newlines and indentation; > + * otherwise the output uses a single line. > + * > * For now, this cannot be used to visit the 'any' type. > */ > -Visitor *json_output_visitor_new(char **result); > +Visitor *json_output_visitor_new(bool pretty, char **result); > > #endif > diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c > index 7010ff8..881c9ee 100644 > --- a/qapi/json-output-visitor.c > +++ b/qapi/json-output-visitor.c > @@ -17,6 +17,7 @@ > struct JsonOutputVisitor { > Visitor visitor; > QString *str; > + bool pretty; > bool comma; > unsigned int depth; > char **result; > @@ -30,10 +31,13 @@ static JsonOutputVisitor *to_jov(Visitor *v) > static void json_output_name(JsonOutputVisitor *jov, const char *name) > { > if (jov->comma) { > - qstring_append(jov->str, ", "); > + qstring_append(jov->str, jov->pretty ? "," : ", "); > } else { > jov->comma = true; > } > + if (jov->pretty && jov->depth) { > + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, ""); > + } > if (name && jov->depth) { > qstring_append_json_string(jov->str, name); > qstring_append(jov->str, ": "); > @@ -57,6 +61,9 @@ static void json_output_end_struct(Visitor *v, void **obj) > > assert(jov->depth); > jov->depth--; > + if (jov->pretty) { > + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, ""); > + } > qstring_append(jov->str, "}"); > jov->comma = true; > } > @@ -85,6 +92,9 @@ static void json_output_end_list(Visitor *v, void **obj) > > assert(jov->depth); > jov->depth--; > + if (jov->pretty) { > + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, ""); > + } > qstring_append(jov->str, "]"); > jov->comma = true; > } > @@ -165,11 +175,12 @@ static void json_output_free(Visitor *v) > g_free(jov); > } > > -Visitor *json_output_visitor_new(char **result) > +Visitor *json_output_visitor_new(bool pretty, char **result) > { > JsonOutputVisitor *v; > > v = g_malloc0(sizeof(*v)); > + v->pretty = pretty; > v->result = result; > *result = NULL; > v->str = qstring_new(); > diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c > index 3c77a61..5073715 100644 > --- a/tests/test-json-output-visitor.c > +++ b/tests/test-json-output-visitor.c > @@ -24,14 +24,17 @@ > > typedef struct TestOutputVisitorData { > Visitor *ov; > + bool pretty; > char *str; > } TestOutputVisitorData; > > static void visitor_output_setup(TestOutputVisitorData *data, > - const void *unused) > + const void *arg) > { > - data->ov = json_output_visitor_new(&data->str); > + const bool *pretty = arg; Could do bool pretty = *(bool *)arg. Matter of taste. Same elsewhere. Blank line between declarations and statements, please. > + data->ov = json_output_visitor_new(*pretty, &data->str); > g_assert(data->ov); > + data->pretty = *pretty; I'd put this line before data->ov = ... > } > > static void visitor_output_teardown(TestOutputVisitorData *data, > @@ -52,8 +55,9 @@ static const char *visitor_get(TestOutputVisitorData *data) > > static void visitor_reset(TestOutputVisitorData *data) > { > + bool pretty = data->pretty; Blank line between declarations and statements, please. > visitor_output_teardown(data, NULL); > - visitor_output_setup(data, NULL); > + visitor_output_setup(data, &pretty); We could pass &data->pretty (teardown leaves it alone), but I guess not relying on that is slightly cleaner. > } > > static void test_visitor_out_int(TestOutputVisitorData *data, > @@ -161,8 +165,9 @@ static void test_visitor_out_struct(TestOutputVisitorData *data, > } > > static void test_visitor_out_struct_nested(TestOutputVisitorData *data, > - const void *unused) > + const void *arg) > { > + const bool *pretty = arg; > int64_t value = 42; > UserDefTwo *ud2; > const char *string = "user def string"; > @@ -192,27 +197,51 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data, > visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort); > > out = visitor_get(data); > - g_assert_cmpstr(out, ==, > - "{" > - "\"string0\": \"forty two\", " > - "\"dict1\": {" > - "\"string1\": \"forty three\", " > - "\"dict2\": {" > - "\"userdef\": {" > - "\"integer\": 42, " > - "\"string\": \"user def string\"" > - "}, " > - "\"string\": \"forty four\"" > - "}, " > - "\"dict3\": {" > - "\"userdef\": {" > - "\"integer\": 42, " > - "\"string\": \"user def string\"" > - "}, " > - "\"string\": \"forty five\"" > - "}" > - "}" > - "}"); > + if (*pretty) { > + g_assert_cmpstr(out, ==, > + "{\n" > + " \"string0\": \"forty two\",\n" > + " \"dict1\": {\n" > + " \"string1\": \"forty three\",\n" > + " \"dict2\": {\n" > + " \"userdef\": {\n" > + " \"integer\": 42,\n" > + " \"string\": \"user def string\"\n" > + " },\n" > + " \"string\": \"forty four\"\n" > + " },\n" > + " \"dict3\": {\n" > + " \"userdef\": {\n" > + " \"integer\": 42,\n" > + " \"string\": \"user def string\"\n" > + " },\n" > + " \"string\": \"forty five\"\n" > + " }\n" > + " }\n" > + "}"); > + } else { > + g_assert_cmpstr(out, ==, > + "{" > + "\"string0\": \"forty two\", " > + "\"dict1\": {" > + "\"string1\": \"forty three\", " > + "\"dict2\": {" > + "\"userdef\": {" > + "\"integer\": 42, " > + "\"string\": \"user def string\"" > + "}, " > + "\"string\": \"forty four\"" > + "}, " > + "\"dict3\": {" > + "\"userdef\": {" > + "\"integer\": 42, " > + "\"string\": \"user def string\"" > + "}, " > + "\"string\": \"forty five\"" > + "}" > + "}" > + "}"); > + } The not-pretty case is unchanged. Good. > qapi_free_UserDefTwo(ud2); > } > > @@ -346,36 +375,49 @@ static void test_visitor_out_null(TestOutputVisitorData *data, > g_assert_cmpstr(out, ==, "null"); > } > > -static void output_visitor_test_add(const char *testpath, > +static void output_visitor_test_add(const char *testpath, bool *data, > void (*test_func)(TestOutputVisitorData *, > const void *)) > { > - g_test_add(testpath, TestOutputVisitorData, NULL, visitor_output_setup, > + g_test_add(testpath, TestOutputVisitorData, data, visitor_output_setup, > test_func, visitor_output_teardown); > } > > int main(int argc, char **argv) > { > + bool plain = false; > + bool pretty = true; > + > g_test_init(&argc, &argv, NULL); > > - output_visitor_test_add("/visitor/json/int", test_visitor_out_int); > - output_visitor_test_add("/visitor/json/bool", test_visitor_out_bool); > - output_visitor_test_add("/visitor/json/number", test_visitor_out_number); > - output_visitor_test_add("/visitor/json/string", test_visitor_out_string); > - output_visitor_test_add("/visitor/json/enum", test_visitor_out_enum); > - output_visitor_test_add("/visitor/json/enum-errors", > + output_visitor_test_add("/visitor/json/int", &plain, > + test_visitor_out_int); > + output_visitor_test_add("/visitor/json/bool", &plain, > + test_visitor_out_bool); > + output_visitor_test_add("/visitor/json/number", &plain, > + test_visitor_out_number); > + output_visitor_test_add("/visitor/json/string", &plain, > + test_visitor_out_string); > + output_visitor_test_add("/visitor/json/enum", &plain, > + test_visitor_out_enum); > + output_visitor_test_add("/visitor/json/enum-errors", &plain, > test_visitor_out_enum_errors); > - output_visitor_test_add("/visitor/json/struct", test_visitor_out_struct); > - output_visitor_test_add("/visitor/json/struct-nested", > + output_visitor_test_add("/visitor/json/struct", &plain, > + test_visitor_out_struct); > + output_visitor_test_add("/visitor/json/struct-nested", &plain, > test_visitor_out_struct_nested); > - output_visitor_test_add("/visitor/json/struct-errors", > + output_visitor_test_add("/visitor/json-pretty/struct-nested", &pretty, > + test_visitor_out_struct_nested); > + output_visitor_test_add("/visitor/json/struct-errors", &plain, > test_visitor_out_struct_errors); > - output_visitor_test_add("/visitor/json/list", test_visitor_out_list); > - output_visitor_test_add("/visitor/json/union-flat", > + output_visitor_test_add("/visitor/json/list", &plain, > + test_visitor_out_list); > + output_visitor_test_add("/visitor/json/union-flat", &plain, > test_visitor_out_union_flat); > - output_visitor_test_add("/visitor/json/alternate", > + output_visitor_test_add("/visitor/json/alternate", &plain, > test_visitor_out_alternate); > - output_visitor_test_add("/visitor/json/null", test_visitor_out_null); > + output_visitor_test_add("/visitor/json/null", &plain, > + test_visitor_out_null); > > g_test_run();