-
Notifications
You must be signed in to change notification settings - Fork 287
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
Move json data #1348
base: main
Are you sure you want to change the base?
Move json data #1348
Conversation
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.
I find this change very scary as we've had horrible bugs trying to move things around in the JSON DOM before. Do you have a benchmark showing this as meaningful?
I don't think moving things around in the JSON DOM is scary as most of the moves are at the end of a scope so there's nothing special about them. However, I do see the argument that re-using a moved-from variable is a possible source of bugs.
This PR improves the performance of |
The thing with DOMs is that their contents are shared around by different components. Moving something that was actually a component of an overall DOM has been a bug we have written a lot of.
What is that in the context of an overall command using this? |
This is only called after you build a package for seconds, minutes or hours. |
If you read the code, you'll notice that this is not an issue here. This kind of bug can be avoided by following best practices regarding const correctness and the single responsibility principle. |
Why we are using json? Can we switch to yaml format? |
This comment was marked as off-topic.
This comment was marked as off-topic.
YAML has its own set of problems. This is a what-about discussion and doesn't belong here. |
No description provided.