Pass SendCoinsRecipient (208 bytes) by reference #10964

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:pass-big-parameters-by-reference changing 4 files +4 −4

practicalswift commented at 3:24 PM on July 31, 2017: contributor

Pass SendCoinsRecipient (208 bytes) by reference.

Avoid passing big parameters by value.
promag commented at 3:38 PM on July 31, 2017: member

These are unrelated structures. How did you came to this change? I mean, aren't there more cases like these?

Anyway, IMO split in 2 commits, one for each structure and remove the sizeof() from PR title and commit messages.
practicalswift commented at 3:44 PM on July 31, 2017: contributor

@promag These two are the only structures over 128 bytes which are passed by value. Let me know if you find any counterexample that falsifies that claim :-)
benma commented at 8:16 PM on August 1, 2017: none

utACK 346d8b096e1a93d673ca6e8090ed3e6f53dd4be7

I'd be happy if you could document how you identified the refactorings you are doing (with the exact command used in case of static checkers) in the commit message.

Do you have a document somewhere that details all of the checks you are doing? Would be handy to have them in one place.
in src/net.cpp:2257 in 346d8b096e outdated
```
2253 | @@ -2254,7 +2254,7 @@ bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<C
2254 |      return fBound;
2255 |  }
2256 |  
2257 | -bool CConnman::Start(CScheduler& scheduler, Options connOptions)
2258 | +bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
```
theuni commented at 9:25 PM on August 1, 2017:

Please don't pass this by reference. This was intentional in case any unique_ptr's end up being passed in. Please just std::move it in init instead.

practicalswift commented at 9:39 PM on August 1, 2017:

Thanks for the clarification! Removing!

benma commented at 6:28 AM on August 2, 2017:

@theuni can you elaborate? Why worry about it when there is no unique_ptr inside now? Even if there was, passing the struct by const reference prevents Connman from taking ownership.

theuni commented at 9:24 PM on August 2, 2017:

@benma Setting the unique_ptr thing aside, generally speaking, I prefer to pass anything with allocated mem by value rather than const reference if I know that it's going to be stored by the caller.

For example, we know that Start() is going to keep a local copies of vSeedNodes/vWhiteBinds/etc. If you pass by const reference, you guarantee the copies. But if you pass by value, you give the caller the opportunity to std::move them in instead.

That said, #10977 changes this to a const reference also. I don't care too strongly, so I'll just shut up about it :)

practicalswift commented at 9:34 PM on August 2, 2017:

@theuni Didn't realize that I changed the same thing in two PRs - sorry :-)
practicalswift force-pushed on Aug 1, 2017
practicalswift renamed this:
~~Pass SendCoinsRecipient (208 bytes) and CConnman::Options (168 bytes) by reference~~
Pass SendCoinsRecipient (208 bytes) by reference
on Aug 1, 2017
practicalswift commented at 3:34 AM on August 2, 2017: contributor

Build error unrelated. May I request a re-build? :-)
Pass SendCoinsRecipient (208 bytes) by const reference d3d946a294
practicalswift force-pushed on Aug 2, 2017
jonasschnelli commented at 9:50 AM on August 3, 2017: contributor

trivial utACK d3d946a294988ca3ab2b2c8848da5b393fbbcf42
jonasschnelli added the label GUI on Aug 3, 2017
jonasschnelli merged this on Aug 15, 2017
jonasschnelli closed this on Aug 15, 2017
jonasschnelli referenced this in commit 64e66bb262 on Aug 15, 2017
jasonbcox referenced this in commit c0dc1ba156 on Mar 26, 2019
PastaPastaPasta referenced this in commit 3be0620dc3 on Sep 19, 2019
PastaPastaPasta referenced this in commit d989d1e75a on Sep 23, 2019
PastaPastaPasta referenced this in commit 8d2e1c1e55 on Sep 24, 2019
PastaPastaPasta referenced this in commit 8496b86542 on Nov 19, 2019
PastaPastaPasta referenced this in commit 9aab729d00 on Dec 9, 2019
PastaPastaPasta referenced this in commit 71725637c2 on Jan 1, 2020
PastaPastaPasta referenced this in commit d6be958afd on Jan 2, 2020
PastaPastaPasta referenced this in commit 728e5acaa5 on Jan 2, 2020
PastaPastaPasta referenced this in commit 0579d5c2c0 on Jan 2, 2020
PastaPastaPasta referenced this in commit 67073009f3 on Jan 2, 2020
ckti referenced this in commit a74bf073f2 on Mar 28, 2021
practicalswift deleted the branch on Apr 10, 2021
gades referenced this in commit 70e29f2fa7 on Jun 28, 2021
gades referenced this in commit 2498667143 on Mar 9, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels