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

builtins.fromJSON: use nlohmann/json parser instead of custom parser #3307

Merged
merged 2 commits into from Jan 10, 2020

Conversation

@yorickvP
Copy link
Contributor

yorickvP commented Jan 9, 2020

Nix already links to nlohmann/json, so we might just as well use it for JSON parsing, instead of a custom parser that is accumulating complexity.

This uses the sax backend to avoid accumulating a lot of data in memory before conversion to nix. I am not sure about the use of new/delete in combination with things that might end up GC'd, but I suspect this is fine because they are all gone before a GC ever happens.

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Jan 9, 2020

Looks good, thanks.

Maybe you can use std::unique_ptr instead of new/delete?

@yorickvP

This comment has been minimized.

Copy link
Contributor Author

yorickvP commented Jan 9, 2020

Done! It's an improvement.

@edolstra edolstra merged commit 72a5075 into NixOS:master Jan 10, 2020
@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Jan 10, 2020

Thanks!

@yorickvP yorickvP deleted the yorickvP:yorickvp/nlohmann-fromJSON branch Jan 10, 2020
dtzWill added a commit to dtzWill/nix that referenced this pull request Jan 10, 2020
builtins.fromJSON: use nlohmann/json parser instead of custom parser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.