Use std::move (C++11) in map and set adaptors where possible. #279
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
| } | ||
| tmp.swap(v); | ||
| v = std::move(v); |
There was a problem hiding this comment.
Some of tests are failed. See https://travis-ci.org/msgpack/msgpack-c/jobs/62302916
It seems that
v = std::move(v)should be
v = std::move(tmp)There was a problem hiding this comment.
Whoops, sorry. I only discovered today that I had previously forgotten this occurrence, and apparently exercised less caution on this one. Uploaded an updated commit, the tests are now passing locally for me.
There was a problem hiding this comment.
Thank you for fixing. I'm on a business trip now. I will merge the PR next week.
In the std::map adaptor, remove the complicated iterator/insert code and go with the cpp11/unordered_map.hpp approach instead. Makes the code more consistent, avoids an extra copy, and the previous complexity was unnecessary since std::map only maps to a single element per key, unlike std::multimap.
redboltz
added a commit
that referenced
this pull request
May 18, 2015
Use std::move (C++11) in map and set adaptors where possible.
|
Thanks! |
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.
Using std::move for map/set is potentially more efficient (depending on the type), but also allows converting to move-only types where previously the types had to have a copy constructor.
For insert(), moving will actually prevent the copy if possible. This is the important part of the patch.
swap() is very similar to a move assignment, but needs to ensure that the
tmpvariable will afterwards contain the original contents of the targetvvariable, which we don't care about but requires a temporary variable for three-way swapping anyway, so it's a bit less optimal.I made the moves dependent on the standard macro for C++11 since there's no need to depend on any msgpack defines in these cases, and in the cpp11/ directory there are no #ifdefs. I did not adapt any code in the tr1/ directory since the TR1 types did not have move constructors or move assignment operators anyway (move got introduced very late in the standardization process).