Skip to content
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

Clean up existing data in move assignment operator #1342

Closed

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Dec 21, 2023

I need some advice on this one from someone with stronger modern C++ knowledge than me.

While trying to find the source of the memory leak in issue #1240, I noticed that the move assignment operator on variant types is not cleaning up any pre-existing data, for example, in Array, there was:

Array &Array::operator=(Array &&other) {
	std::swap(opaque, other.opaque);
	return *this;
}

If opaque already contained a valid array, this seems like it would swap it for the data in other.opaque, without ever cleaning up the pre-existing data in opaque.

Does this make sense? Or, am I just misunderstanding these new fangled rvalue references and move operations?

Marking as a draft because I don't know if this makes sense

(PS: This is NOT the source of the memory leak in #1240 as my PR doesn't fix it.)

@dsnopek dsnopek requested a review from a team as a code owner December 21, 2023 20:17
@dsnopek dsnopek marked this pull request as draft December 21, 2023 20:17
@dsnopek dsnopek added this to the 4.x milestone Dec 21, 2023
@dsnopek dsnopek added the bug This has been identified as a bug label Dec 21, 2023
@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 21, 2023

Or, is this ok because std::swap() will have put the contents of opaque into other.opaque and since other is temporary, we can depend on its destructor leading to the data getting cleaned up?

@mihe
Copy link
Contributor

mihe commented Dec 21, 2023

Or, is this ok because std::swap() will have put the contents of opaque into other.opaque and since other is temporary, we can depend on its destructor leading to the data getting cleaned up?

This is correct. The changes in this PR shouldn't be needed.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 21, 2023

Ah, ok, thanks!

@dsnopek dsnopek closed this Dec 21, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants