Always swap in generated move operator - #2055
Merged
dsnopek merged 1 commit intoSep 8, 2026
Merged
Conversation
Collaborator
Author
|
It took a little work to get the MRP from #2048 working, but I've been able to reproduce the issue, and this PR does seem to fix it for me! |
Ivorforce
approved these changes
Sep 8, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, this is the code we generate for
Dictionary's move operator:It's simply overwriting the
opaquewith data from a newDictionary, without cleaning up the data from the oldDictionary(whereas the usual idiom is to swap the internal data withp_other, which will get cleaned up whenp_otheris destructed).It looks like this comes from #656 - there was an issue where destructing
Dictionary's could lead to error spam, because of anERR_FAIL_NULL()if aDictionaryis destructed with null data. (This code is still present inDictionary::unref().) This null could get set in the move constructor ofDictionarybecause it was swappingopaquevalue with a zero'd out one, so a fix was added to just copy rather than the usual swap.However, it's only in the move constructor where a
Dictionarycould end up with null data, but the fix made the same change to move operator!Anyway, this PR removes the fix from the move operator, while leaving it in place for the move constructor
I haven't tested yet, but I think this fixes #2048