Skip to content

Pass SendCoinsRecipient (208 bytes) by reference#10964

Merged
jonasschnelli merged 1 commit intobitcoin:masterfrom
practicalswift:pass-big-parameters-by-reference
Aug 15, 2017
Merged

Pass SendCoinsRecipient (208 bytes) by reference#10964
jonasschnelli merged 1 commit intobitcoin:masterfrom
practicalswift:pass-big-parameters-by-reference

Conversation

@practicalswift
Copy link
Copy Markdown
Contributor

@practicalswift practicalswift commented Jul 31, 2017

Pass SendCoinsRecipient (208 bytes) by reference.

Avoid passing big parameters by value.

Copy link
Copy Markdown
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

practicalswift commented Jul 31, 2017

@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
Copy link
Copy Markdown

benma commented Aug 1, 2017

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.

Comment thread src/net.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification! Removing!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theuni Didn't realize that I changed the same thing in two PRs - sorry :-)

@practicalswift practicalswift force-pushed the pass-big-parameters-by-reference branch from 346d8b0 to ffc7b6d Compare August 1, 2017 21:40
@practicalswift practicalswift changed the title Pass SendCoinsRecipient (208 bytes) and CConnman::Options (168 bytes) by reference Pass SendCoinsRecipient (208 bytes) by reference Aug 1, 2017
@practicalswift
Copy link
Copy Markdown
Contributor Author

Build error unrelated. May I request a re-build? :-)

@practicalswift practicalswift force-pushed the pass-big-parameters-by-reference branch from ffc7b6d to d3d946a Compare August 2, 2017 08:52
@jonasschnelli
Copy link
Copy Markdown
Contributor

trivial utACK d3d946a

@jonasschnelli jonasschnelli merged commit d3d946a into bitcoin:master Aug 15, 2017
jonasschnelli added a commit that referenced this pull request Aug 15, 2017
d3d946a Pass SendCoinsRecipient (208 bytes) by const reference (practicalswift)

Pull request description:

  Pass `SendCoinsRecipient` (208 bytes) by reference.

  Avoid passing big parameters by value.

Tree-SHA512: 504791f1b1c73badbc276db13b83e39695298d7d82a9db0e48d54e7ef02f1a8d276b0adfdece1ba1130cc214e2f0fa9a3100b5359d0ca0fe96558d3c9a786e6e
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 26, 2019
Summary:
Completes T559

Backport of PR10964
bitcoin/bitcoin#10964

Test Plan:
make check
test_runner.py

Reviewers: jasonbcox, Fabien, deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Subscribers: teamcity, schancel

Differential Revision: https://reviews.bitcoinabc.org/D2701
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 9, 2019
d3d946a Pass SendCoinsRecipient (208 bytes) by const reference (practicalswift)

Pull request description:

  Pass `SendCoinsRecipient` (208 bytes) by reference.

  Avoid passing big parameters by value.

Tree-SHA512: 504791f1b1c73badbc276db13b83e39695298d7d82a9db0e48d54e7ef02f1a8d276b0adfdece1ba1130cc214e2f0fa9a3100b5359d0ca0fe96558d3c9a786e6e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020
d3d946a Pass SendCoinsRecipient (208 bytes) by const reference (practicalswift)

Pull request description:

  Pass `SendCoinsRecipient` (208 bytes) by reference.

  Avoid passing big parameters by value.

Tree-SHA512: 504791f1b1c73badbc276db13b83e39695298d7d82a9db0e48d54e7ef02f1a8d276b0adfdece1ba1130cc214e2f0fa9a3100b5359d0ca0fe96558d3c9a786e6e
@practicalswift practicalswift deleted the pass-big-parameters-by-reference branch April 10, 2021 19:32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 28, 2021
d3d946a Pass SendCoinsRecipient (208 bytes) by const reference (practicalswift)

Pull request description:

  Pass `SendCoinsRecipient` (208 bytes) by reference.

  Avoid passing big parameters by value.

Tree-SHA512: 504791f1b1c73badbc276db13b83e39695298d7d82a9db0e48d54e7ef02f1a8d276b0adfdece1ba1130cc214e2f0fa9a3100b5359d0ca0fe96558d3c9a786e6e
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 9, 2022
d3d946a Pass SendCoinsRecipient (208 bytes) by const reference (practicalswift)

Pull request description:

  Pass `SendCoinsRecipient` (208 bytes) by reference.

  Avoid passing big parameters by value.

Tree-SHA512: 504791f1b1c73badbc276db13b83e39695298d7d82a9db0e48d54e7ef02f1a8d276b0adfdece1ba1130cc214e2f0fa9a3100b5359d0ca0fe96558d3c9a786e6e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants