coins: Simplify std::move to ternary in coins.cpp
#30656
Conversation
See related discussions: * bitcoin#28280 (comment) * bitcoin#28280 (comment) * bitcoin#17487 (comment) And reproducer that demonstrates the behavior of all 3 cases: * https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. |
l0rinc
changed the title
coins.cppcoins.cpp
| if (cursor.WillErase(*it)) { | ||
| // Since this entry will be erased, | ||
| // we can move the coin into us instead of copying it | ||
| entry.coin = std::move(it->second.coin); | ||
| } else { | ||
| entry.coin = it->second.coin; | ||
| } | ||
| cursor.WillErase(*it) | ||
| ? (entry.coin = std::move(it->second.coin)) | ||
| : (entry.coin = it->second.coin); |
There was a problem hiding this comment.
Can you explain what problem switching an if to ternary is solving? Linking to 3 discussion threads and one external gist, which may be deleted at any time seems not helpful to reviewers.
There was a problem hiding this comment.
You're right, added the following to the description of the pr:
To avoid repetition and make the diff trivial between the two branches (calling the copy vs move constructors), this expression was originally written as a ternary, which unfortunately introduced an additional copy operation (and was reverted to a verbose if statement with a comment).
This change attempts to restore the signal to noise ratio of such a simple expression while retaining its performance.
There was a problem hiding this comment.
I don't understand the reasoning to be honest. This seems to be just a cosmetic refactor, and I am not sure if it is really clearer, or if this was desired by other reviewers. The commit message also makes no mention of why the move constructors are now defaulted, or why that is required.
There was a problem hiding this comment.
Also, the current code is clearer for me, especially due to the comment.
There was a problem hiding this comment.
The comment duplicates what the code already stated, but seems there's an agreement that this isn't an improvement, so I'll just close the PR.
5 participants
Split out of #30643
To avoid repetition and make the diff trivial between the two branches (calling the copy vs move constructors), this expression was originally written as a ternary, which unfortunately introduced an additional copy operation (and was reverted to a verbose if statement with a comment).
This change attempts to restore the signal to noise ratio of such a simple expression while retaining its performance.
See related discussions:
And reproducer that demonstrates the behavior of all 3 cases: