-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Fix json::update() with merge_objects==true error #5017
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
base: develop
Are you sure you want to change the base?
Conversation
f36e5d6 to
58d0073
Compare
…el Hwangbo <[email protected]> Signed-off-by: Daniel Hwangbo <[email protected]>
58d0073 to
0d1eb84
Compare
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @DanielHwangbo |
|
Maybe related: #4813 |
Signed-off-by: Daniel Hwangbo <[email protected]>
|
Hi nlohmann, I have fixed the Amalgamation error and would like to request a code review again! |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @DanielHwangbo |
gregmarr
left a comment
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.
You have a LOT of unrelated formatting changes in this file. Did your editor do an auto-format?
| m_data.m_value.object->operator[](it.key()) = it.value(); | ||
|
|
||
| // Insert/Overwrite logic | ||
| auto& ref = m_data.m_value.object->operator[](it.key()); |
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.
What is the reason for this change?
gregmarr
left a comment
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 recommend reverting all of the whitespace changes and the changes at line 3491 to introduce ref. Then you also need to amalgamate and add the file to the commit. I don't know if it didn't work before, or if you didn't add the file, but there are no changes to the amalgamated header.
| { | ||
| it2->second.update(it.value(), true); | ||
|
|
||
| // Repair the child's subtree. |
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 think the comment should go inside the #if.
| auto& ref = m_data.m_value.object->operator[](it.key()); | ||
| ref = it.value(); | ||
|
|
||
| // We must traverse the entire tree of 'this' and reset everyone's parent |
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 think the comment should go inside the #if.
Description
This PR fixes a crash when using
update()on anordered_jsonobject whenJSON_DIAGNOSTICSis enabled.The Issue
When
update()inserts new elements into anordered_jsonobject, the underlyingstd::vectormay reallocate. This invalidates them_parentpointers of existing children, causingassert_invariantto fail immediately because the children point to the old memory address.The Fix
I have updated the
update()function to callset_parents()after insertion operations when diagnostics are enabled. This ensures that if the container moves in memory, all children are correctly updated to point to the new parent address.Reproduction Code