Skip to content

Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Sep 4, 2018
Merged

Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249

laanwj merged 1 commit into from
Sep 4, 2018

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented May 16, 2018 •
edited
Loading

Make objects in range declarations immutable by default.

Rationale:

  • Immutable objects are easier to reason about.
  • Prevents accidental or hard-to-notice change of value.

@Empact
Copy link
Contributor

Empact commented May 17, 2018

How about also including the range-for changes from #12158?

@practicalswift
Copy link
Contributor Author

practicalswift commented May 17, 2018 •
edited
Loading

@Empact I think it better to do (a subset of) the #12158 changes in follow-up PRs to keep this PR as mechanical and easy-to-review as possible. Basically what @laanwj suggested in #12158 (comment) :-)

This PR should hopefully be trivial to review :-)

@Empact
Copy link
Contributor

Empact commented May 17, 2018 •
edited
Loading

Yeah, I like it as-is, just suggesting since the & additions wouldn't add much if anything to the line count, so it could be viewed as a form of compression.

Anyway, you have my Tested ACK 3299ed7.
make, make check and test/test_bitcoin ran green locally.

@practicalswift
Copy link
Contributor Author

practicalswift commented May 24, 2018

Rebased!

@ken2812221
Copy link
Contributor

ken2812221 commented Jun 7, 2018

utACK 0143466

This was referenced Jun 7, 2018
Closed
Closed
@Empact
Copy link
Contributor

Empact commented Jun 10, 2018

utACK 0143466

promag
promag reviewed Jun 10, 2018
View reviewed changes
Copy link
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.

I've only made some comments where I think the iterated type can be const &. Can you review the whole change if you agree with such change?

src/miner.cpp Outdated
Copy link
Contributor

@promag promag Jun 10, 2018

Choose a reason for hiding this comment

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

const CTxMemPool::txiter&?

src/net.cpp Outdated
Copy link
Contributor

@promag promag Jun 10, 2018

Choose a reason for hiding this comment

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

nit, s/auto/CNode*?

src/net_processing.cpp Outdated
Copy link
Contributor

@promag promag Jun 10, 2018

Choose a reason for hiding this comment

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

const uint256&?

src/net_processing.cpp Outdated
Copy link
Contributor

@promag promag Jun 10, 2018

Choose a reason for hiding this comment

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

Same as above.

This was referenced Jun 14, 2018
Merged
Merged
@practicalswift practicalswift force-pushed the for-const branch 3 times, most recently from 3e16523 to 9a4655f Compare June 14, 2018 21:03
@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 14, 2018

@promag Thanks for reviewing! Feedback addressed. Please re-review :-)

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/policy/rbf.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Could be a reference too for consistency with src/miner.cpp.

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/qt/bitcoin.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Whitespace

Copy link
Contributor Author

@practicalswift practicalswift Jun 14, 2018

Choose a reason for hiding this comment

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

I'm not sure I understand what whitespace change you're suggesting? :-)

Copy link
Contributor

@Empact Empact Jun 15, 2018

Choose a reason for hiding this comment

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

Just that the developer-notes prefer WalletModel*. Fine to ignore.

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/rpc/blockchain.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Same for this file

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/txmempool.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Same

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Reference?

Empact
Empact reviewed Jun 14, 2018
View reviewed changes
src/wallet/walletdb.cpp Outdated
Copy link
Contributor

@Empact Empact Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: References?

@Empact
Copy link
Contributor

Empact commented Jun 14, 2018

All nits, just calling out handling where inconsistent.

@sipa
Copy link
Member

sipa commented Jun 14, 2018

utACK 9a4655fd15e632d95651e4681936f8ea13457ae1

These changes should be obviously safe. Adding const to variable declarations should never introduce problems (as long as the result compiles). The same is true for converting const variables to references.

This was referenced Jun 15, 2018
Merged
Merged
Closed
@practicalswift
Copy link
Contributor Author

practicalswift commented Jul 12, 2018

Rebased! :-)

promag
promag reviewed Jul 12, 2018
View reviewed changes
Copy link
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.

Comments regarding auto usage.

src/wallet/wallet.cpp Outdated
Copy link
Contributor

@promag promag Jul 12, 2018

Choose a reason for hiding this comment

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

Could use const COutput& coin?

src/validation.cpp Outdated
Copy link
Contributor

@promag promag Jul 12, 2018

Choose a reason for hiding this comment

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

Most common usage when iterating mapBlockIndex is

for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)

And there is also the above:

for (const BlockMap::value_type& entry : mapBlockIndex)

(I prefer the 1st).

src/wallet/wallet.cpp Outdated
Copy link
Contributor

@promag promag Jul 12, 2018

Choose a reason for hiding this comment

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

Could use const CTxIn& input (most common usage).

@practicalswift practicalswift force-pushed the for-const branch 2 times, most recently from 279caa6 to ddd0045 Compare July 12, 2018 16:54
@practicalswift
Copy link
Contributor Author

practicalswift commented Jul 12, 2018

@promag Thanks for reviewing. Feedback addressed. Please re-review :-)

This was referenced Jul 25, 2018
Closed
Closed
Merged
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Aug 22, 2018
Merged
@practicalswift
Make objects in range declarations immutable by default. Avoid unnece…
f34c8c4 
…ssary copying of objects in range declarations.
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 27, 2018

Rebased! Please re-review :-)

@practicalswift practicalswift mentioned this pull request Aug 31, 2018
Closed
@DrahtBot DrahtBot mentioned this pull request Aug 31, 2018
Closed
@ken2812221
Copy link
Contributor

ken2812221 commented Sep 4, 2018

utACK f34c8c4

@laanwj laanwj merged commit f34c8c4 into bitcoin:master Sep 4, 2018
laanwj added a commit that referenced this pull request Sep 4, 2018
@laanwj
Merge #13249: Make objects in range declarations immutable by default…
5c24d3b 
…. Avoid unnecessary copying of objects in range declarations.

f34c8c4 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)

Pull request description:

  Make objects in range declarations immutable by default.

  Rationale:
  * Immutable objects are easier to reason about.
  * Prevents accidental or hard-to-notice change of value.

Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
@Bushstar Bushstar mentioned this pull request Oct 11, 2018
Merged
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 11, 2019
@practicalswift @fpelliccioni
Make objects in range declarations immutable by default. Avoid unnece…
33322f2 
…ssary copying of objects in range declarations.

Summary:
Make objects in range declarations immutable by default.

Backport of Bitcoin Core PR13249
bitcoin/bitcoin#13249

(D4191 done again)

Test Plan:
```
make check-all
```

Reviewers: Fabien, #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D4221
@practicalswift practicalswift deleted the for-const branch April 10, 2021 19:35
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 19, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13249: Make objects in range declarations immutable by …
ff5a947 
…default. Avoid unnecessary copying of objects in range declarations.

f34c8c4 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)

Pull request description:

  Make objects in range declarations immutable by default.

  Rationale:
  * Immutable objects are easier to reason about.
  * Prevents accidental or hard-to-notice change of value.

Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 9, 2022
@laanwj @gades
Merge bitcoin#13249: Make objects in range declarations immutable by …
d891336 
…default. Avoid unnecessary copying of objects in range declarations.

f34c8c4 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)

Pull request description:

  Make objects in range declarations immutable by default.

  Rationale:
  * Immutable objects are easier to reason about.
  * Prevents accidental or hard-to-notice change of value.

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

Reviewers

4 more reviewers

@Empact Empact Empact left review comments

@theuni theuni theuni left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@practicalswift @Empact @ken2812221 @sipa @theuni @maflcko @promag @laanwj @fanquake @DrahtBot