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

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

0using 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:

 0diff --git a/src/txmempool.h b/src/txmempool.h
 1index bda812b42f..ebfcf36e11 100644
 2--- a/src/txmempool.h
 3+++ b/src/txmempool.h
 4@@ -486,7 +486,7 @@ public:
 5     mutable CCriticalSection cs;
 6     indexed_transaction_set mapTx GUARDED_BY(cs);
 7 
 8-    typedef indexed_transaction_set::nth_index<0>::type::iterator txiter;
 9+    using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
10     std::vector<std::pair<uint256, txiter> > vTxHashes; //!< All tx witness hashes/entries in mapTx, in random order
11 
12     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