Make item_location copyable #31743
Merged
Make item_location copyable #31743
Conversation
This file contains 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
The copy constructor for item_location was deleted. I think this dated from a time when it was implemented via unique_ptr, but now it uses shared_ptr and has a clone method. This only serves to make code unnecessarily complex. Reinstate the copy constructor (doing the same thing as clone), remove clone, and update other code accordingly. In particular, this dramatically simplifies parts of player_activity and inventory_entry, since they can now have default copy construction and assignment.
ifreund
added
[C++]
ifreund
reviewed
Jun 22, 2019
|
Move is still faster than copy, and a reasonable choice when semantically logical. They will do no harm. |
|
I believe there are some cases where we don't want to move (see this article) but I agree it's really not a big deal, and probably beneficial in most cases. |
|
As that article points out, such uses can be found via compiler warnings. We can catch them when we start using gcc 9 at some point :). |
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.
Summary
SUMMARY: None
Purpose of change
The copy constructor for
item_locationwas deleted. I think this dated from a time when it was implemented viaunique_ptr, but now it usesshared_ptrand has aclonemethod.This only serves to make code unnecessarily complex.
Describe the solution
Reinstate the copy constructor (doing the same thing as
clone), removeclone, and update other code accordingly.In particular, this dramatically simplifies parts of
player_activityandinventory_entry, since they can now have default copy construction and assignment.Additional context
A necessary diversion during my work towards repairing some of the unsafety of
item_location.