Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format

[René Scharfe <l.s.r@xxxxxx>]

Am 07.06.2018 um 16:12 schrieb git@xxxxxxxxxxxxxxxxx:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> +/*
> + * Add comma if we have already seen a member at this level.
> + */
> +static inline void maybe_add_comma(struct json_writer *jw)
> +{
> +	if (!jw->open_stack.len)
> +		return;

This is impossible.  maybe_add_comma() is only called directly after
assert_in_object() and assert_in_object(), which abort if open_stack 
is empty.

> +	if (jw->first_stack.buf[jw->first_stack.len - 1] == '1')
> +		jw->first_stack.buf[jw->first_stack.len - 1] = '0';
> +	else
> +		strbuf_addch(&jw->json, ',');

You only need a binary flag to indicate if a comma is needed, not a
stack.  We need a comma at the current level if and only if we already
wrote a child item.  If we finish a level then we do need a comma at the
previous level because we just wrote a sub-object or sub-array there.
And that should cover all cases.  Am I missing something?

I get a sense of déjà vu, by the way. :)

Here's what I mean:
---
 json-writer.c | 17 ++++++-----------
 json-writer.h | 13 +++++--------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/json-writer.c b/json-writer.c
index f35ce1912a..14a4e89188 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -5,7 +5,7 @@ void jw_init(struct json_writer *jw)
 {
 	strbuf_reset(&jw->json);
 	strbuf_reset(&jw->open_stack);
-	strbuf_reset(&jw->first_stack);
+	jw->need_comma = 0;
 	jw->pretty = 0;
 }
 
@@ -13,7 +13,6 @@ void jw_release(struct json_writer *jw)
 {
 	strbuf_release(&jw->json);
 	strbuf_release(&jw->open_stack);
-	strbuf_release(&jw->first_stack);
 }
 
 /*
@@ -69,7 +68,7 @@ static inline void begin(struct json_writer *jw, char ch_open, int pretty)
 	strbuf_addch(&jw->json, ch_open);
 
 	strbuf_addch(&jw->open_stack, ch_open);
-	strbuf_addch(&jw->first_stack, '1');
+	jw->need_comma = 0;
 }
 
 /*
@@ -99,12 +98,10 @@ static inline void assert_in_array(const struct json_writer *jw)
  */
 static inline void maybe_add_comma(struct json_writer *jw)
 {
-	if (!jw->open_stack.len)
-		return;
-	if (jw->first_stack.buf[jw->first_stack.len - 1] == '1')
-		jw->first_stack.buf[jw->first_stack.len - 1] = '0';
-	else
+	if (jw->need_comma)
 		strbuf_addch(&jw->json, ',');
+	else
+		jw->need_comma = 1;
 }
 
 static inline void fmt_double(struct json_writer *jw, int precision,
@@ -397,8 +394,6 @@ void jw_end(struct json_writer *jw)
 	char ch_open;
 	int len;
 
-	if (jw->open_stack.len != jw->first_stack.len)
-		BUG("jwon-writer: open_ and first_ stacks out of sync");
 	if (!jw->open_stack.len)
 		BUG("json-writer: too many jw_end(): '%s'", jw->json.buf);
 
@@ -406,7 +401,7 @@ void jw_end(struct json_writer *jw)
 	ch_open = jw->open_stack.buf[len];
 
 	strbuf_setlen(&jw->open_stack, len);
-	strbuf_setlen(&jw->first_stack, len);
+	jw->need_comma = 1;
 
 	if (jw->pretty)
 		strbuf_addch(&jw->json, '\n');
diff --git a/json-writer.h b/json-writer.h
index af9c2612f8..c437462ba8 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -59,18 +59,15 @@ struct json_writer
 	struct strbuf open_stack;
 
 	/*
-	 * Another simple stack of the currently open array and object
-	 * forms.  This is used in parallel to "open_stack" (same length).
-	 * It contains a string of '1' and '0' where '1' indicates that
-	 * the next child-element seen will be the first child (and does
-	 * not need a leading comma).
+	 * Indicates if the next child item needs a leading comma.
+	 * Only the first item of arrays and objects doesn't need one.
 	 */
-	struct strbuf first_stack;
+	unsigned int need_comma:1;
 
-	int pretty;
+	unsigned int pretty:1;
 };
 
-#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, 0 }
+#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, 0, 0 }
 
 void jw_init(struct json_writer *jw);
 void jw_release(struct json_writer *jw);
-- 
2.17.1