fix: don't force a copy of std::string fname when moving is an option #189
Conversation
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
Taking this parameter by const reference forces us to copy it (because we know we're going to store it). Taking it by r-value reference would suggest that we _might_ take ownership over it and would also force the user to make a copy if they wish to retain the original value. Taking this parameter by value however clearly gives us ownership of its content without forcing a copy if it's implicit conversion from `const char*` or explicitly handed over to us by the user via std::move.
|
This looks nice. After checking CI results, I will merge it. |
ToruNiina
approved these changes
Jun 3, 2022
|
? |
|
Your change has not been merged yet. And this change is worth merging. Also, it seems that all the CI tests pass. Could you restore the branch and reopen this Pull Req? If you don't want to be listed at contributors for some reason, then I will add this manually. Could you tell me what should I do? |
|
I have no idea what happened. Maybe when I was cleaning up branches in some other unrelated repos I accidentally deleted this one as well? Anyway, I restored the branch and PR. Apologies for the inconvenience and weirdness. |
|
Thank you! |
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.
Taking this parameter by const reference forces us to copy it (because
we know we're going to store it). Taking it by r-value reference would
suggest that we might take ownership over it and would also force the
user to make a copy if they wish to retain the original value.
Taking this parameter by value however clearly gives us ownership of its
content without forcing a copy if it's implicit conversion from
const char*or explicitly handed over to us by the user via std::move.Also note that without this the
std::movecall already present inparseis effectively a NOP. (I.e. it calls the const r-value ref constructor of
std::string, which doesn't exist, so falls back to const l-value ref.).