Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii <eliz@gnu.org>]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 29 May 2018 15:59:06 +0100
> 
> So I found this other thread
> 
>   https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00667.html
> 
> where plist support for json.c is briefly requested and discussed, but
> there didn't seem to be an overwhelming argument against it, so I had
> another look at the file and it didn't seem very hard or problematic to
> implement.
> 
> Anyway, I had a go.

Thanks, please see a few comments below.

> +@vindex json-serialize-use-plists
> +@cindex serializing plists as json
> +  For the serialization function, the variable
> +@var{json-serialize-use-plists} controls the converse process,

json-serialize-use-plists is the literal name of a variable, so it
should have the @code markup, not @var.

> +resolving the ambiguity when a list is found in the Lisp object to
> +serialize. If @code{nil}, its default, the list is interpreted as an
            ^^
Two spaces between sentences, please.

> +          if ( EQ (Vjson_serialize_use_plists, Qt) ) {
> +            key_symbol = XCAR (tail);
> +            tail = XCDR(tail);
> +            CHECK_CONS (tail);
> +            value = XCAR (tail);
> +            if (EQ (tail, li.tortoise)) circular_list (lisp);
> +          } else {
> +            Lisp_Object pair = XCAR (tail);

Our style is to write

  if (something)
    {
       then-part;
    }
  else
    {
       else-part;
    }

Also, please don't leav any whitespace between the opening parenthesis
and the following character, and similarly with closing parens.  IOW,
this:

   if ( EQ (Vjson_serialize_use_plists, Qt) )

should be this instead:

   if (EQ (Vjson_serialize_use_plists, Qt))

> +              json_object_foreach (json, key_str, value)
> +                {
> +                  /* No idea if using AUTO_STRING and Fconcat for
> +                     making keywords is idiomatic, but seems to work
> +                     nicely */
> +                  AUTO_STRING (colon, ":");
> +                  Lisp_Object key =
> +                    Fintern (CALLN (Fconcat, colon, json_build_string (key_str))
> +                             , Qnil);
> +                  result = Fcons (key, result); /* build the plist as
> +                                                   value-key since
> +                                                   we're going to
> +                                                   reverse it in the
> +                                                   end.*/
> +                  result = Fcons (json_to_lisp (value, object_type), result);
> +                }

Here I'd use intern_1 instead, it would allow you to avoid
unnecessarily consing Lisp objects.  (Yes, I realize that the same
comment applies to the existing code.)

> +               /* If non-nil use plists instead of alists in json-serialize.*/);
                               ^
Comma is missing there.