Skip to content

refactor: Replace m_params with chainman.GetParams()#25830

Merged
maflcko merged 1 commit intobitcoin:masterfrom
aureleoules:2022-08-replace-m_params
Oct 19, 2022
Merged

refactor: Replace m_params with chainman.GetParams()#25830
maflcko merged 1 commit intobitcoin:masterfrom
aureleoules:2022-08-replace-m_params

Conversation

@aureleoules
Copy link
Contributor

@aureleoules aureleoules commented Aug 12, 2022 •
edited
Loading

Fixes a TODO introduced in #24595.
Removes m_params from CChainState class and replaces it with m_chainman.GetParams().

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 13, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25172 (refactor: use std:: prefix for std lib funcs by fanquake)
  • #20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)
  • #9245 (Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Aug 13, 2022
Closed
Merged
Closed
Merged
Merged
@aureleoules aureleoules force-pushed the 2022-08-replace-m_params branch from 83efe63 to 672fdf3 Compare September 16, 2022 12:18
@DrahtBot DrahtBot mentioned this pull request Sep 19, 2022
Merged
@aureleoules aureleoules force-pushed the 2022-08-replace-m_params branch from 672fdf3 to 5d3f98d Compare October 10, 2022 15:44
@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

Why the close?

@aureleoules
Copy link
Contributor Author

aureleoules commented Oct 13, 2022

@MarcoFalke I was asked by @adamjonas to close this during coredev.

@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

It is fixing a TODO, so if that is no longer applicable, the TODO should be removed

@adamjonas
Copy link
Member

adamjonas commented Oct 13, 2022

@MarcoFalke the request to close was based on lack of review and, therefore, the implied lack of support. If you'd be willing to review, @aureleoules can reopen.

@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

sure

@aureleoules aureleoules reopened this Oct 13, 2022
@aureleoules
Copy link
Contributor Author

aureleoules commented Oct 13, 2022

Note that I could have reduced the code diff by renaming params to m_params but since params is not a class member anymore I think its more confusing.

@DrahtBot DrahtBot mentioned this pull request Oct 18, 2022
Closed
maflcko
maflcko reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK 5d3f98d 🌎

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 5d3f98d27879cd6d84b8590e947336e8d09613ed 🌎
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjJ8Qv/ct7XpfKidLyXHj8qvtiMkjeM7lH4rtiwWVOi23PmFTl3UXN+CJZwuXlt
P7onWWziczOZIJzJVVwFKgbOegIfG2QNWlVpSWsgQRYXYdCi8wRyqmsChxKUNdQ/
+Yl2ny0CMpoEORl1rD8/wFdvBhpKbkVDl/dbCKLmz1KUxLfFDWPfcZ2VdyCrtoKr
+jXmBUtYVGciQaD/MK638lK/AgBEy1GzbBnnBZKg8oKi729FsEzFY9C7mGFT7OMc
RMn2hRKMdl9Vjz5ffBVp7DXC5tHTG6RJMZAIilI6xdw77BX5qFK5+HqGsOh/27sx
sn6mA0kP+tqPxV4/lD6PgtoYpbmQN9afxVJ8zFLCOM/TU4gIbQzlApQ1iM4/x+jp
m5E3lE0OAmWgns8YQ1ui2VkbcQhPU+0OfzoZC/3VPH5fGF5L2fW0aFsdgy2jT4Ph
pIFZjGbXvI8m/WEok1Rps4ZEKcIqkFjK0q3l29gJILPHZNtagEf4s4zqWc7czmtd
DiVFo5K1
=Udt+
-----END PGP SIGNATURE-----

@maflcko
Copy link
Member

maflcko commented Oct 19, 2022

In the future it might be best not to add refactoring TODOs?

@maflcko maflcko merged commit a97791d into bitcoin:master Oct 19, 2022
@aureleoules aureleoules deleted the 2022-08-replace-m_params branch November 2, 2022 10:52
@bitcoin bitcoin locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aureleoules @DrahtBot @maflcko @adamjonas @fanquake