[FFmpeg-devel] [PATCH 2/2] ffprobe: add basic JSON print format.

[Alexander Strasser]

Hi!

Clément Bœsch wrote:
> On Sat, Sep 03, 2011 at 03:23:51PM +0200, Alexander Strasser wrote:
[...]
> >   What do you think of implementing it like
> > 
> > static char *json_escape_str(const char *s)
> > {
> >     static const char json_escape[] = {'"', '\\', '\b', '\f', '\n', '\r', '\t'};
> >     static const char json_subst[]  = {'"', '\\',  'b',  'f',  'n',  'r',  't'};
> > 
> > that?
> > 
> >   Would get rid of the pointer-to-char-array indirection and the
> > trailing ASCII NUL that terminates the C strings.
> > 
> >   Other advantages are:
> >   - arrays are private to the only function that uses them
> >   - the array initializers emphasize the symmetry of the two arrays
> > 
> 
> Sure ok. Changed.

  Thanks for changing it and still adding the zero termination. I
was a bit to eager, but of course we need the zero terminator in
the first string because we are using strchr() to search it. 

> > > +static void json_print_fmt(const char *key, const char *fmt, ...)
> > > +{
> > > +    int len;
> > > +    char *raw_str = NULL, *value_esc = NULL, *key_esc;
> > 
> >   NIT: Why is value_esc inited to NULL but key_esc isn't? Or vice versa.
> > 
> 
> value_esc is set to NULL in case of error. key_esc is computed in case of
> error (send "end:" label), so it doesn't matter.

  You are right, my bad. I did not look close enough, because of the
possible jump the value_esc pointer must be initialized to NULL.

> BTW, I moved that code outside in another patch, so I'll send yet another
> patch set (3 patchs) in a minute (sorry for changing my mind again).

  No problem! I looked at your new patch series and I like the way you
shuffled the code around. It makes the output writer implementations
simpler and less error prone. Also it is a good idea to export the
asprintf functionality as that will be useful in other places too.

[...]

  Alexander