From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 23 Mar 2018 14:13:08 GMT
Subject: Re: [PATCH v2] json_writer: new routines to create data in JSON format
Message-ID: <d6ef7034-351f-c1ef-a5d7-8ed684078ba7@jeffhostetler.com>
In-Reply-To: <xmqq8talnmts.fsf@gitster-ct.c.googlers.com>



On 3/21/2018 5:25 PM, Junio C Hamano wrote:
> git@jeffhostetler.com writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Add basic routines to generate data in JSON format.
> 
> And the point of having capability to write JSON data in our
> codebase is...?
> 
>> diff --git a/json-writer.c b/json-writer.c
>> new file mode 100644
>> index 0000000..89a6abb
>> --- /dev/null
>> +++ b/json-writer.c
>> @@ -0,0 +1,321 @@
>> +#include "cache.h"
>> +#include "json-writer.h"
>> +
>> +static char g_ch_open[2]  = { '{', '[' };
>> +static char g_ch_close[2] = { '}', ']' };
> 
> What's "g_" prefix?

Global.

Sorry, very old habits.

>> +
>> +/*
>> + * Append JSON-quoted version of the given string to 'out'.
>> + */
>> +static void append_quoted_string(struct strbuf *out, const char *in)
>> +{
>> +	strbuf_addch(out, '"');
>> +	for (/**/; *in; in++) {
>> +		unsigned char c = (unsigned char)*in;
> 
> It is clear enough to lose /**/, i.e.
> 
> 	for (; *in; in++) {
> 
> but for this one. I wonder if
> 
> 	unsigned char c;
> 	strbuf_addch(out, '"');
> 	while ((c = *in++) != '\0') {
>   		...
> 
> is easier to follow, though.

either way is fine.  will fix.



>> +static inline void begin(struct json_writer *jw, int is_array)
>> +{
>> +	ALLOC_GROW(jw->levels, jw->nr + 1, jw->alloc);
>> +
>> +	jw->levels[jw->nr].level_is_array = !!is_array;
>> +	jw->levels[jw->nr].level_is_empty = 1;
> 
> An element of this array is a struct that represents a level, and
> everybody who accesses an element of that type knows it is talking
> about a level by the field that has the array being named as
> .levels[] (also [*1*]).  In such a context, it is a bit too loud to
> name the fields with level_$blah.  IOW,
> 
> 	struct json_writer_level
> 	{
> 		unsigned is_array : 1;
> 		unsigned is_empty : 1;
> 	};

make sense.  will fix.

>> +struct json_writer_level
>> +{
>> +	unsigned level_is_array : 1;
>> +	unsigned level_is_empty : 1;
>> +};
>> +
>> +struct json_writer
>> +{
>> +	struct json_writer_level *levels;
>> +	int nr, alloc;
>> +	struct strbuf json;
>> +};
> 
> [Footnote]
> 
> *1* I personally prefer to call an array of things "thing[]", not
>      "things[]", because then you can refer to an individual element
>      e.g. "thing[4]" and read it as "the fourth thing".
> 
>      Unless the code often treats an array as a whole, that is, in
>      which case, things[] is OK as you'll be calling the whole thing
>      with the plural name (e.g. call that function and give all the
>      things by passing things[]).
> 
>      In this case, one level instance is an element of a stack, and
>      the code would be accessing one level at a time most of the
>      time, so "writer.level[4].is_empty" would read more naturally
>      than "writer.levels[4].level_is_empty".

yeah, that makes sense.

Thanks
Jeff