UnPackTo disable merge by default #7527
Conversation
|
@RWauson FYI |
There was a problem hiding this comment.
I think it is admirable that you are supporting both the "merge" and "reuse" use cases, but like I said in the linked issue, "reuse" was the intended use case from the start, and "merge" is a unintended niche use case.
Since supporting code complicates the code (generates more, and slower code), I personally would vote for only supporting reuse. But up to you.
|
My only reason to include it as an option is because I am switching defaults (I don't know when it was originally switched away from what we wanted). So having the parameter gives people an easy way to go back to the old way. |
|
That old way was never intended though, the amount of people relying on that behavior should be minimal. I'd be fine not supporting that. |
|
Yeah, I can remove the shrink to fit part. I also removed the |
dbaileychess
added
the
release-notes
…e memory leak The existing code relies on UnPackTo to merge the existing state with the new incoming state. However, this behavior was changed in google/flatbuffers#7527 and now leads to memory leak. Ignore the commit description there as it is outdated. During the review, they decided to not have the old behavior as an option. PiperOrigin-RevId: 484798857
Fixes: #7458
Adds a new field
bool _merge = falsetoUnPackTothat dictates if the receiving object's existing state is merged with the incoming state or not. When_merge == false, it will reset any pointer in the receiving object to nullptr if there is nothing to unpack. For vectors, the vector is cleared (resized to 0) and then shrunk to reclaim memory.Previously, the merging of states was on by default, but I don't think that is the correct default value. See #7458 for background discussion. So this change makes merging off by default, and have to be explicitly opted in.
This is tested in two new tests that test with and without merging.