Avoid copies in range-for loops and add a warning to detect them #13480

pull theuni wants to merge 2 commits into bitcoin:master from theuni:no-for-copies changing 7 files +10 −9

theuni commented at 5:55 PM on June 15, 2018: member

Following-up on #13241, which was itself a follow-up of #12169.

See title. Fixing these would otherwise be a continuous process, adding the warning should keep them from cropping up.

Note that the warning seems to be Clang-only for now.
cleanup: avoid hidden copies in range-for loops 466e16e0e8
build: add warning to detect hidden copies in range-for loops d92204c900
theuni commented at 5:58 PM on June 15, 2018: member

Most of these aren't really significant or are outside of critical paths, but this one is definitely worth fixing.
MarcoFalke added the label Build system on Jun 15, 2018
sipa commented at 6:18 PM on June 15, 2018: member

I'm confused about the removal of consts, and in general by why you're touching loops with a non-reference variable at all - whenever there is no reference type involved, a copy should always be created?

in src/miner.cpp:212 in d92204c900

 208 | @@ -209,7 +209,7 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost
 209 |  //   segwit activation)
 210 |  bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package)
 211 |  {
 212 | -    for (const CTxMemPool::txiter it : package) {
 213 | +    for (CTxMemPool::txiter it : package) {

MarcoFalke commented at 6:28 PM on June 15, 2018:

I'd slightly prefer if you added a

using const_txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;

and used that instead, since that seemed to be the intention here.

theuni commented at 6:30 PM on June 15, 2018:

Agreed!

MarcoFalke commented at 11:33 PM on June 17, 2018:

Actually, iterators are always const for multindex sets, so I'd slightly prefer if that was clarified in the header file:

diff --git a/src/txmempool.h b/src/txmempool.h
index bda812b42f..ebfcf36e11 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -486,7 +486,7 @@ public:
     mutable CCriticalSection cs;
     indexed_transaction_set mapTx GUARDED_BY(cs);
 
-    typedef indexed_transaction_set::nth_index<0>::type::iterator txiter;
+    using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
     std::vector<std::pair<uint256, txiter> > vTxHashes; //!< All tx witness hashes/entries in mapTx, in random order
 
     struct CompareIteratorByHash {

MarcoFalke commented at 3:52 PM on June 18, 2018:

That can be done in a separate pull, though. Has nothing to do with your changes.

MarcoFalke commented at 6:28 PM on June 15, 2018: member

utACK d92204c900d55ebaf2af5c900162b3c2c8c296e2
theuni commented at 6:29 PM on June 15, 2018: member
From the pull-request to llvm that added the feature, it warns about:
for (const Foo &x : Foos), where the range Foos does not return a copy. This warning will suggest using the non-reference type so the copy is obvious.
for (const Foo x : Foos), where the range Foos does return a reference, but is copied into x. This warning will suggest using the reference type to prevent a copy from being made.
for (const Bar &x : Foos), where Bar is constructed from Foo. In this case, suggest using the non-reference "const Bar" to indicate a copy is intended to be made, or "const Foo &" to prevent a copy from being made. @sipa The iterators here are the second case. I've always copied iterators by value rather than by reference, but I can't recall the reasoning for that habit now. I figured the consts were misleading, as a copy could still change the underlying element, so I just removed 'em. Happy to make them const references instead, if you'd prefer.
MarcoFalke deleted a comment on Jun 15, 2018
practicalswift commented at 8:01 PM on June 15, 2018: contributor

utACK d92204c900d55ebaf2af5c900162b3c2c8c296e2

Thanks for making this fix persistent using -Wrange-loop-analysis. Automation is our friend :-)
Empact commented at 9:15 PM on June 15, 2018: member

utACK d92204c I also like the const_txiter suggestion
promag commented at 8:48 PM on June 23, 2018: member

utACK d92204c.
laanwj commented at 2:36 PM on June 24, 2018: member

utACK d92204c900d55ebaf2af5c900162b3c2c8c296e2
laanwj merged this on Jun 24, 2018
laanwj closed this on Jun 24, 2018
laanwj referenced this in commit 31145a3d7c on Jun 24, 2018
PastaPastaPasta referenced this in commit 60e0ee4a99 on Jul 7, 2020
PastaPastaPasta referenced this in commit 85f633e26b on Jul 7, 2020
PastaPastaPasta referenced this in commit c298c1ef77 on Jul 8, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12169 Avoid temporary copies in C++11 ranged-based for loops.#13241 scripted-diff: Avoid temporary copies when looping over std::map