Subject: Re: bug#70007: [PATCH] native JSON encoder

From: Eli Zaretskii <[email-masked]>
Date: Sat, 30 Mar 2024 16:22:57 +0300

> From: Mattias Engdegård <[email-masked]>
> Date: Sat, 30 Mar 2024 12:41:31 +0100
> Cc: [email-masked],
>  [email-masked]
> 
> 29 mars 2024 kl. 07.04 skrev Eli Zaretskii <[email-masked]>:
> 
> > OK.  FTR, I'm not in favor of validation of unibyte strings, I just
> > suggest that we treat them as plain-ASCII: pass them through without
> > any validation, leaving the validation to the callers.
> 
> Actually we are more or less forced to validate unibyte strings as long as the serialiser returns multibyte. Which we agree that it probably shouldn't, but I'd first like to take some time to ensure that returning unibyte won't break anything.

Yes, I was writing about the state of affairs when we change the
serializer to emit unibyte strings.

> Thank you for pushing the new JSON parser to master. I've rebased my patch and cleaned it up a bit, and it now removes all uses of Jansson from json.c. Since the change involves removing some Windows-specific code, perhaps you would like to check that it still compiles on that platform, if you have the time?

It builds and passes the tests, thanks.  But I have a couple of minor
nits below.

> +static struct symset_tbl *
> +make_symset_table (int bits, struct symset_tbl *up)
> +{
> +  int maxbits = min (SIZE_WIDTH - 2 - (word_size < 8 ? 2 : 3), 32);
> +  if (bits > maxbits)
> +    error ("out of memory");	/* Will never happen in practice.  */

This should be a call to memory_full, no?  Or if we must signal this
error here, at least make the error message more specific: mention
JSON or somesuch.

> +{
> +  double x = XFLOAT_DATA (f);
> +  if (isinf (x) || isnan (x))
> +    signal_error ("not a finite number", f);

I'd prefer a more specific error message here, like

  JSON does not allow Inf or NaN

Last, but not least: should we have a json-available-p function that
always returns non-nil, for better backward-compatibility?  Otherwise,
code out there might decide to use json.elm which is not what we want.