GNU bug report logs - #32793
27.0.50; json-parse-string doesn't have the equivalent of json.el's json-array-type

Previous Next

Full log

Message #17 received at 32793 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: mail <at> xuchunyang.me, 32793 <at> debbugs.gnu.org
Subject: Re: bug#32793: 27.0.50; json-parse-string doesn't have the equivalent
 of json.el's json-array-type
Date: Fri, 12 Apr 2019 12:22:44 +0300

> Cc: mail <at> xuchunyang.me, 32793 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Fri, 12 Apr 2019 03:34:05 +0300
> 
> > Sounds like a simple extension of the existing code, so patches are
> > welcome to implement this feature.
> 
> Very well, patch attached.
> 
> What do you think?

Thanks, some comments below.

> +enum json_array_type {
> +  json_array_array,
> +  json_array_list
> +};
> +
>  struct json_configuration {
>    enum json_object_type object_type;
> +  enum json_array_type array_type;
>    Lisp_Object null_object;
>    Lisp_Object false_object;
>  };

Can't say I like this conversion of Lisp symbols into C enumerations.
I'd rather we used symbols (Qarray, Qlist, etc.), but I can understand
why you did this as json.c already did for the other keyword values.

> @@ -521,7 +527,7 @@ static void
>  json_parse_args (ptrdiff_t nargs,
>                   Lisp_Object *args,
>                   struct json_configuration *conf,
> -                 bool configure_object_type)
> +                 bool configure_types)

If we are renaming this argument, let's do a better job: I think its
name should have been parse_object_types.

Come to think of this: why do we need this boolean at all?  The
callers which don't want :object-type parsed will ignore the result
anyway, so it sounds like something we could just toss.

> +          case json_array_list:
> +            {
> +              result = Qnil;
> +              for (ptrdiff_t i = 0; i < size; ++i)
> +                result = Fcons (json_to_lisp (json_array_get (json, i), conf),
> +                                result);
> +              result = Fnreverse (result);

If you cons the list back to front, you can avoid the Fnreverse call,
which will make this faster.

Also, please insert a call to rarely_quit into the loop, as JSON
vectors could be quite large, AFAIU.

Finally, this needs documentation update: the doc strings of
json-parse-string and json-parse-buffer, NEWS, and the ELisp manual.

Thanks again for working on this.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.