-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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.
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
Refactor: Move consts to their correct translation units #17383
Refactor: Move consts to their correct translation units #17383
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
I don't know if it makes sense to move the consts to |
Concept ACK. I think it would make sense longer-term if modules managed their own arguments, but for now, that's not the case so init.cpp is the right place for them. If you take a look at the constants I've moved init init.cpp, you'll see:
|
Concept ACK Could do |
Concept ACK
Yes, I think there's two ideas in conflict here. One is to move the defaults to implementation files (instead of having them clutter headers), which is a good thing in itself, and the other is to do parsing where the values are needed instead of having I think it's ok to do this first. |
Concept ACK. |
Nicely put. Definite concept ACK on doing this (in future PRs) |
I thought the benefit of the current approach is to fail as soon as possible? |
Moving the parsing code to another module doesn't mean it can't fail on init (see e.g. the wallet init parsing) |
Ah ok, my comment was because of this bit. Breaking the initialization sounds good to me. |
Are we supposed to care about AppVeyor failures? Is there a way to rerun them when they fail? |
I don't want scope to creep too much. It made sense to me to change const to constexpr when moving a constant from the header file to the implementation file, but for the simple moves within files I kept the definitions the same. |
I suggest ignoring the appveyor result until #17384 (comment) is merged |
I've restarted. There's a Rebuild PR button there (maybe if you have permissions?). |
Yesss I agree I didn't literally mean where the values are needed, but the module where the values are needed, Of course it's better to do as much of parsing upfront as possible. Failing on argument parsing when threads have already been spun up is inconvenient. It's exactly what is wrong at the moment, why parsing can't fail in |
2335e65
to
0a0ac29
Compare
rebased |
rebased on master |
0a0ac29
to
3ba3f11
Compare
ACK 3ba3f11 -- |
3ba3f11
to
e9ea95a
Compare
I've simplified this PR to remove any moves to init.cpp. I think the discussion above about start-up and separating components is interesting, but wasn't really the point of this PR. This PR now only does the following:
I've also removed changing const to constexpr and adding doxygen comments. Reviewing this PR should be trivial:
|
I disagree that review of this is trivial. Let's take a look at the first move ( Also, I don't really understand the motivation to move compile time constants of trivial types from the header to the cpp file. This has no effect on compile time and it shouldn't matter where they live. Not to mention that this (potentially silently) conflicts with open pull requests. |
ACK on moving net_processing consts from validation to net_processing. Not sure about the rest. |
No, the second value is a timeout on any socket activity, not just pings. It should live in net. PING is an application-level message and exists in net_processing.
The header file is the interface. The .cpp file is the implementation. Constants that are only needed in the implementation should live in the .cpp file, and the interface should be kept as minimal as possible. I don't see why this is controversial in any way.
One reason that we have merge conflicts is that there's very tight coupling between files in this repo. If symbols were declared in the tightest possible scope there would be far fewer conflicts. I picked this PR up again because I'm making a branch in net_processing and found myself touching files in validation and net. That shouldn't happen! Please read the other comments on this PR. You're the only person who's objected to this. I've pared this down and down to satisfy you by removing the changes in init.cpp, removing doxygen comments, and changing constexprs back to consts so this is now just move only. |
Fair enough, Concept ACK |
ACK e9ea95a -- patch looks correct |
ACK e9ea95a 🚉 Show signature and timestampSignature:
Timestamp of file with hash |
…on units e9ea95a [net processing] Move all const declarations to top of net_processing.cpp (John Newbery) 507b36d [validation] Move all const declarations to top of validation.h (John Newbery) 0109622 [validation] Move validation-only consts to validation.cpp (John Newbery) b8580ca [net processing] Move net processing consts to net_processing.cpp (John Newbery) Pull request description: Following the main.cpp split, there are still some constants in the wrong places, eg net_processing constants in validation.h. Move them all to their rightful homes. At the same time, make them constexpr. Also move all const declarations to the top of their files, and ensure that they all have doxygen comments. ACKs for top commit: practicalswift: ACK e9ea95a -- patch looks correct MarcoFalke: ACK e9ea95a 🚉 Tree-SHA512: 44d81da73c7be01e1d36b939789d793f297d3b94f84ea4e7ac853c621cc7054b5a05c7c9e7b83db506db44c16f344541be8f240d955694211e53a84c32b0d2c5
Updated upstream bitcoin/bitcoin#17383 for the auxpow header size constraints.
Summary: > Following the main.cpp split, there are still some constants in the wrong places, eg net_processing constants in validation.h. Move them all to their rightful homes. At the same time, make them constexpr. > > Also move all const declarations to the top of their files, and ensure that they all have doxygen comments. This is a backport of Core [[bitcoin/bitcoin#17383 | PR17383]] [1/4] bitcoin/bitcoin@b8580ca Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8976
Summary: This is a backport of Core [[bitcoin/bitcoin#17383 | PR17383]] [2/4] bitcoin/bitcoin@0109622 Depends on D8976 `UNDOFILE_CHUNK_SIZE` and `BLOCKFILE_CHUNK_SIZE` were moved to blockdb.h in D7579 `MAX_DISCONNECTED_TX_POOL_SIZE` was moved to txmempool.h on D1667 `MAX_FEE_ESTIMATION_TIP_AGE` is unused since D2784 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8977
Summary: This is a backport of Core [[bitcoin/bitcoin#17383 | PR17383]] [3/4] bitcoin/bitcoin@507b36d Depends on D8977 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8978
….cpp Summary: This is a backport of Core [[bitcoin/bitcoin#17383 | PR17383]] [4/4] bitcoin/bitcoin@e9ea95a Depends on D8978 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8980
merge bitcoin#16117, bitcoin#18358, bitcoin#17383, bitcoin#21052, bitcoin#14424, bitcoin#15159, bitcoin#14689, bitcoin#14978, partial bitcoin#16908, bitcoin#14978, bitcoin#13932: Auxillary Backports
Following the main.cpp split, there are still some constants in the wrong places, eg net_processing constants in validation.h. Move them all to their rightful homes. At the same time, make them constexpr.
Also move all const declarations to the top of their files, and ensure that they all have doxygen comments.