-
Notifications
You must be signed in to change notification settings - Fork 138
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
Fix Cadence 1.0 migration of dictionary values when using atree inlined data #3316
Fix Cadence 1.0 migration of dictionary values when using atree inlined data #3316
Conversation
This commit fixes Cadence 1.0 migration when using atree inlined data. See issue: #3288 Previously, MigrateNestedValue() migrates dictionary by using readonly iterator and migrating values in place. This commit migrates keys first using readonly iterator and then migrates values using mutable iterator.
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.
Great fix!
As discussed in the review meeting, splitting up the migration into a pass migrating the keys using read-only iteration, and another pass migrating the values using mutable iteration, makes sense 👍
I only realized this after the call but: To have the atree-register inlining branch not drift too much, let's apply the refactor to master, even if it is not needed.
@fxamacker I'm working on porting this to master. After that we can improve it a bit more (TODOs we discussed), update the atree register inlining branch, and then add the tests with #3314 |
@turbolent Thanks for approving and code review of this PR yesterday! In addition to passing all tests in PR #3314, I also verified this morning that it resolves "slab not found error" using input file |
@fxamacker Awesome! Thank you for checking this, it's great to see the fix works as expected 👌 |
@fxamacker Ported this to master in #3318, and added the improvements we discussed yesterday. PTAL 🙏 |
Closes #3288
This PR fixes
MigrateNestedValue()
for dictionary values.Previously,
MigrateNestedValue()
migrates dictionary by using readonly iterator and migrating values in place.This PR migrates keys first using read-only iterator and then migrates values using mutable iterator.
More details at the test/reproducer:
master
branchFiles changed
in the Github PR explorer