Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unparse json feature #648

Merged
merged 11 commits into from May 24, 2020
Merged

Unparse json feature #648

merged 11 commits into from May 24, 2020

Conversation

ekaitz-zarraga
Copy link
Contributor

@ekaitz-zarraga ekaitz-zarraga commented May 23, 2020

I made a JSON encoder functionality called unparse-json in order to solve #590 .
It's supposed to be the counterpart of parse-json, using the same types.

I'm not sure if the implementation is best in many cases. It's probably easy to improve in many points, that's why I separated everything in self-contained commits that are easier to review.

There are also some things I'm not sure about:

  • JSON doesn't really limit the size of the numbers you can store on it, so I'm not sure if it's reasonable to add support for Bignums. Maybe we can convert them to float?
  • I didn't add support for rationals in order to make the precision loss during the float conversion explicit and force the user to remind that JSON doesn't support them automatically.
  • There are two implementations of the fixnum encoder, one uses alloca (bb1fdbb) and the other (3745c16) doesn't but it's uglier in my opinion.
  • Not sure about flonum implementation neither. You probably come up with something better.
  • Portability is also a concern.

So it's a working solution but we'll probably need to make some changes.

I hope it's useful.

Thanks

@ashinn
Copy link
Owner

ashinn commented May 24, 2020

We should probably change this to json-read and json-write in line with SRFI 180.

@ekaitz-zarraga
Copy link
Contributor Author

ekaitz-zarraga commented May 24, 2020

Well, srfi 180 doesn't work like this. It can handle files larger than memory (this lib can't) and it's designed to read from a port, so this different naming can help separate concepts.
If you are sure, I can arrange a new commit.

Did you have the chance to check the internals?

@ashinn
Copy link
Owner

ashinn commented May 24, 2020

Actually, I specifically wrote this lib to process a data set that couldn't fit in memory.
However, I relied on the fact that the json was written one object per line.

It's worth extending to read directly from ports for convenience, although now I see
SRFI 180 allows generators (a terrible idea) and a tree fold (good idea, but still
difficult to support from C).

lib/chibi/json.c Outdated
sexp unparse_json (sexp ctx, sexp self, sexp obj) {
sexp_gc_var1(res);
sexp_gc_preserve1(ctx, res);
res = SEXP_NULL;
Copy link
Owner

@ashinn ashinn May 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default return value should be SEXP_VOID.

Copy link
Contributor Author

@ekaitz-zarraga ekaitz-zarraga May 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 28f3641
Thanks!

@ashinn
Copy link
Owner

ashinn commented May 24, 2020

I'll probably switch to ports as a follow-up, but this is fine for now.

@ashinn ashinn merged commit 2315a11 into ashinn:master May 24, 2020
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants