-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
simplify extccomp.nim json logic via jsonutils; fix #18084 #18100
Merged
Araq
merged 11 commits into
nim-lang:devel
from
timotheecour:pr_fix_18084_betterRun_nims
Jun 3, 2021
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b3e804a
simplify extccomp.nim json logic via jsonutils
timotheecour b7fb8b9
works
timotheecour 8531079
BuildCache
timotheecour 9678a78
simpler
timotheecour fd1f370
simpler2
timotheecour 4b1713f
simpler3
timotheecour 23ea41b
simplify4
timotheecour 5780d66
fix #18084
timotheecour 3f72dd7
simplify further
timotheecour b70267f
fixup
timotheecour 168c57a
workaround for bootstrap that can be removed after updating csources_…
timotheecour File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. If you want to improve this, give us a "jsonbuilder" module first. One that doesn't repeat json's mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Araq are you referring to timotheecour#745 (serialize/deserialize directly from string instead of from intermediate JsonNode) or to something else? If so, yes I agree timotheecour#745 is needed down the line for performance in similar cases but it would be a 1 liner change on top of this PR once that feature lands. If you're worried about performance I can make a benchmark but I don't see a reason why this PR would create any meaningful overhead.
the name jsonbuilder sounds like it'd be instead similar to the
json.%*
operator (%* {"age":35, "pi":3.1415}
), but that's worse than what I'm doing as there's no spec/type-safety and serialization/deserialization would need more synchronization, unlike this PR withBuildCache
(which is analog to a protobuf proto)this PR is a big code simplification as evidenced by the diff