Use std::move() instead of copying/removing in TxMemPool #8313

pull rodentrabies wants to merge 1 commits into bitcoin:master from rodentrabies:tx-delete changing 7 files +36 −37

rodentrabies commented at 3:43 PM on July 7, 2016: contributor

Fix for #8099 . Don't know if the solution is clean enough, but still. Tried to make changes as small as possible, thus the raw pointer and the optional parameter.
sipa commented at 4:01 PM on July 7, 2016: member

Concept ACK
theuni commented at 5:44 PM on July 7, 2016: member

Moving from a de-referenced shared_ptr is kinda wonky. How about passing a pointer to a list of shared_ptr instead, which you would grab using GetSharedTx rather than GetTx. Then as a bonus, you can use splice() rather than moving.
rodentrabies commented at 6:28 PM on July 7, 2016: contributor

@theuni, you mean replace list<CTransaction> with list<shared_ptr<CTransaction>> at the very top in removeForReorg(), or just in RemoveStaged()
theuni commented at 7:19 PM on July 7, 2016: member

@yurizhykin I mean everywhere this PR adds a std::list<CTransaction>* removed param, use std::list<std::shared_ptr<CTransaction> >* removed instead.
rodentrabies commented at 7:21 PM on July 7, 2016: contributor

OK, will fix it. Thanks for guidelines.
sipa commented at 7:22 PM on July 7, 2016: member

Concept ACK on passing shared_ptr instead.
Use std::move() instead of copying/removing in TxMemPool 4622aea433
rodentrabies force-pushed on Jul 8, 2016
rodentrabies commented at 11:28 PM on July 8, 2016: contributor

Okay, I'm not 100% sure I did exactly as I was told to, but as list for storing deleted transactions is passed down the stack from some top level point (in main.cpp and some tests), I had to replace list<CTransaction> declarations with list<shared_ptr<const CTransaction>> ones everywhere at those points. Also removed redundant list<CTransaction> result local variable declaration in CTxMemPool::removeConflicts().
laanwj added the label Refactoring on Jul 19, 2016
sipa commented at 12:48 PM on August 25, 2016: member

I completely forgot about this PR when writing #8515. That one goes further though, and is based on top of #7946 (which this would need rebasing on).

Do you still plan on working on this?
rodentrabies commented at 7:00 PM on August 28, 2016: contributor

Well, as long as it is done way better in #8515, I should probably just close this one.
rodentrabies closed this on Aug 31, 2016
rodentrabies deleted the branch on Mar 15, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#8099 Use std::move when deleting CTransactions from CTxMempool