coins: Simplify std::move to ternary in coins.cpp #30656

1. 
   paplorinc commented at 3:13 pm on August 14, 2024: contributor

   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:

   • #28280 (review)
   • #28280 (review)
   • #17487 (review)

   And reproducer that demonstrates the behavior of all 3 cases:

   • https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717
2. coins: Simplify coin moves to ternary

   See related discussions:
   * https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676800505
   * https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801434
   * https://github.com/bitcoin/bitcoin/pull/17487#discussion_r402037193
   
   And reproducer that demonstrates the behavior of all 3 cases:
   * https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717

   c98d57900e
3. 
   DrahtBot commented at 3:13 pm on August 14, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

4. DrahtBot added the label UTXO Db and Indexes on Aug 14, 2024
5. paplorinc renamed this:
   coins: Simplify moves to ternary in `coins.cpp`
   coins: Simplify std::move to ternary in `coins.cpp`
   on Aug 14, 2024
6. in src/coins.cpp:202 in c98d57900e

   204-                } else {
   205-                    entry.coin = it->second.coin;
   206-                }
   207+                cursor.WillErase(*it)
   208+                    ? (entry.coin = std::move(it->second.coin))
   209+                    : (entry.coin = it->second.coin);
   

   ---

   ---

   

   maflcko commented at 3:21 pm on August 14, 2024:

   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.

   ---

   

   paplorinc commented at 3:40 pm on August 14, 2024:

   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.

   ---

   

   TheCharlatan commented at 4:07 pm on August 14, 2024:

   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.

   ---

   

   brunoerg commented at 10:54 pm on August 14, 2024:

   @TheCharlatan +1

   Also, the current code is clearer for me, especially due to the comment.

   ---

   

   paplorinc commented at 6:23 am on August 15, 2024:

   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.
7. maflcko changes_requested
8. paplorinc closed this on Aug 15, 2024

   ---

Contributors
paplorinc DrahtBot maflcko TheCharlatan brunoerg

Labels
UTXO Db and Indexes