From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Philipp Stephani
> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 19 Sep 2017 08:18:14 +0000
>
> Here's a newer version of the patch. The only significant differen= ce is that now the Lisp values for JSON null
> and false are :null and :false, respectively. Using a dedicated symbol= for :null reduces the mental overhead of
> the triple meaning of nil (null, false, empty list), and is more futur= e-proof, should we ever want to support lists.
Thanks, a few comments below.
> +static _Noreturn void
> +json_parse_error (const json_error_t *error)
> +{
> +=C2=A0 xsignal (Qjson_parse_error,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0list5 (build_string (error-&= gt;text), build_string (error->source),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 make_n= atnum (error->line), make_natnum (error->column),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 make_n= atnum (error->position)));
> +}
I think error->source could include non-ASCII characters, in which
case you need to use make_specified_string with its last argument
non-zero, not build_string, which has its own ideas about when to
produce a multibyte string.
> +static=C2=A0 _GL_ARG_NONNULL ((2)) Lisp_Object
> +lisp_to_json_1 (Lisp_Object lisp, json_t **json)
> +{
> +=C2=A0 if (VECTORP (lisp))
> +=C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 ptrdiff_t size =3D ASIZE (lisp);
> +=C2=A0 =C2=A0 =C2=A0 eassert (size >=3D 0);
> +=C2=A0 =C2=A0 =C2=A0 if (size > SIZE_MAX)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 xsignal1 (Qoverflow_error, build_pure_c_s= tring ("vector is too long"));
I don't think you can allocate pure storage at run time, only at dump time.=C2=A0 (There are more of this elsewhere in the patch.)OK, will be fixed in the next version.=C2= =A0
> +=C2=A0 /* LISP now must be a vector or hashtable.=C2=A0 */
> +=C2=A0 if (++lisp_eval_depth > max_lisp_eval_depth)
> +=C2=A0 =C2=A0 xsignal0 (Qjson_object_too_deep);
This error could mislead: the problem could be in the nesting of
surrounding Lisp being too deep, and the JSON part could be just fine.
<= /blockquote>Agreed, but I think it's better to use = lisp_eval_depth here because it's the total nesting depth that could ca= use stack overflows.=C2=A0
> +=C2=A0 Lisp_Object string
> +=C2=A0 =C2=A0 =3D make_string (buffer_and_size->buffer, buffer_and= _size->size);
This is arbitrary text, so I'm not sure make_string is appropriate.
Could the text be a byte stream, i.e. not human-readable text?=C2=A0 If so,=
do we want to create a unibyte string or a multibyte string here?It should always be UTF-8.=C2=A0
> +=C2=A0 insert_from_string (string, 0, 0, SCHARS (string), SBYTES (str= ing), false);
Hmmm... if you want to insert the text into the buffer, you need to
make sure it has the right representation.=C2=A0 What kind of text is this?=
It probably should be decoded.
In any case, going through a string sounds gross.=C2=A0 You should insert the text directly into the gap, like we do in a couple of places
already.=C2=A0 See insert_from_gap and its users, and maybe also
decode_coding_gap.OK, I'll have to= check that, but it sounds doable.=C2=A0
> +DEFUN ("json-parse-string", Fjson_parse_string, Sjson_parse= _string, 1, 1, NULL,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0doc: /* Parse the JSON STRING into a Lisp = object.
> +This is essentially the reverse operation of `json-serialize', wh= ich
> +see.=C2=A0 The returned object will be a vector or hashtable.=C2=A0 I= ts elements
> +will be `:null', `:false', t, numbers, strings, or further ve= ctors and
> +hashtables.=C2=A0 If there are duplicate keys in an object, all but t= he
> +last one are ignored.=C2=A0 If STRING doesn't contain a valid JSO= N object,
> +an error of type `json-parse-error' is signaled.=C2=A0 */)
> +=C2=A0 (Lisp_Object string)
> +{
> +=C2=A0 ptrdiff_t count =3D SPECPDL_INDEX ();
> +=C2=A0 check_string_without_embedded_nulls (string);
> +
> +=C2=A0 json_error_t error;
> +=C2=A0 json_t *object =3D json_loads (SSDATA (string), 0, &error)= ;
Doesn't json_loads require the string to be encoded in some particular<= br> encoding?=C2=A0 If so, passing it our internal representation might not be<= br> TRT.
> +=C2=A0 /* First, parse from point to the gap or the end of the access= ible
> +=C2=A0 =C2=A0 =C2=A0portion, whatever is closer.=C2=A0 */
> +=C2=A0 ptrdiff_t point =3D d->point;
> +=C2=A0 ptrdiff_t end;
> +=C2=A0 {
> +=C2=A0 =C2=A0 bool overflow =3D INT_ADD_WRAPV (BUFFER_CEILING_OF (poi= nt), 1, &end);
> +=C2=A0 =C2=A0 eassert (!overflow);
> +=C2=A0 }
> +=C2=A0 size_t count;
> +=C2=A0 {
> +=C2=A0 =C2=A0 bool overflow =3D INT_SUBTRACT_WRAPV (end, point, &= count);
> +=C2=A0 =C2=A0 eassert (!overflow);
> +=C2=A0 }
Why did you need these blocks in braces?To be able to reuse the "overflow" name/
> +(provide 'json-tests)
> +;;; json-tests.el ends here
IMO, it would be good to test also non-ASCII text in JSON objects.