refactor: Misc scheduler cleanups by maflcko · Pull Request #19090 · bitcoin/bitcoin

maflcko · 2020-05-28T13:07:17Z

This accumulates a bunch of cleanup that was long overdue, but I haven't yet gotten around to address. Specifically, but not limited to:

Remove unused code, documentation and includes
Upgrade to doxygen documentation

Please refer to the individual commits for more details.

No longer needed after commit d61f2bb

The common setup is included in virtually all tests, so it should be as slim as possible.

practicalswift · 2020-05-28T13:16:02Z

Concept ACK

ajtowns

Concept ACK

src/scheduler.h

After commit d0ebd93 the scheduler itself no longer cares if the serviceQueue is run in a std::thread or boost::thread. Change the documentation to std::thread because we switched to C++11.

jnewbery

Code review ACK fae1ef5e7

src/scheduler.h

maflcko · 2020-05-28T18:26:42Z

Removed some more code as requested by @jnewbery

This helps understanding the code at the call site without having to look up the name of the argument or the default value.

maflcko · 2020-05-28T23:31:38Z

Reverted back to the previous version for a smaller diff

jnewbery · 2020-05-29T02:17:29Z

reACK fa197f1d1119346b8980cf1a8b34ceee07f30a6a

only change is renaming StopImmediately() to stop() (presumably so StopWhenDrained() can be removed in a follow-up PR.

Reverted back to the previous version for a smaller diff

Good. My original suggestion was to remove StopWhenDrained() in a follow-up. No need to do everything in one PR.

…ion declarations and calls cc29d1e [tools] Update clang-format config (John Newbery) Pull request description: In some cases, running clang-format has made code _less_ readable by joining declarations and calls for functions with many arguments into very long lines. For example: ``` - size_t getQueueInfo(std::chrono::system_clock::time_point &first, - std::chrono::system_clock::time_point &last) const; + size_t getQueueInfo(std::chrono::system_clock::time_point& first, std::chrono::system_clock::time_point& last) const; ``` (bitcoin/bitcoin#19090 (comment)) This change to clang-format would allow arguments/parameters for func declarations/calls to be split over multiple lines, aligned with the opening parens. It does not force args/params to be on new lines (that setting is `BinPackParameters : true`). ACKs for top commit: MarcoFalke: ACK cc29d1e fine with me practicalswift: ACK cc29d1e Tree-SHA512: a62474925e71aaff41bdce7960fd5ffd64317da810f694d8084080b054708cf71c2ab2ce3111db5a9260d1c1f9e02d59a2ecb5543b1b6172ce085cb42432160a

Can easily be reviewed with the git options --ignore-all-space --word-diff-regex=. -U0

maflcko · 2020-06-21T10:07:10Z

Force pushed last commit, but should be trivial to re-review with git range-diff bitcoin-core/master --word-diff-regex=. A B

jnewbery · 2020-06-22T13:27:52Z

utACK fa8337f

…e function declarations and calls cc29d1e [tools] Update clang-format config (John Newbery) Pull request description: In some cases, running clang-format has made code _less_ readable by joining declarations and calls for functions with many arguments into very long lines. For example: ``` - size_t getQueueInfo(std::chrono::system_clock::time_point &first, - std::chrono::system_clock::time_point &last) const; + size_t getQueueInfo(std::chrono::system_clock::time_point& first, std::chrono::system_clock::time_point& last) const; ``` (bitcoin#19090 (comment)) This change to clang-format would allow arguments/parameters for func declarations/calls to be split over multiple lines, aligned with the opening parens. It does not force args/params to be on new lines (that setting is `BinPackParameters : true`). ACKs for top commit: MarcoFalke: ACK cc29d1e fine with me practicalswift: ACK cc29d1e Tree-SHA512: a62474925e71aaff41bdce7960fd5ffd64317da810f694d8084080b054708cf71c2ab2ce3111db5a9260d1c1f9e02d59a2ecb5543b1b6172ce085cb42432160a

fa8337f clang-format scheduler (MarcoFalke) fa3d41b doc: Switch scheduler to doxygen comments (MarcoFalke) fac43f9 scheduler: Replace stop(true) with StopWhenDrained() (MarcoFalke) fa9cca0 doc: Remove unused documentation about unimplemented features (MarcoFalke) fab2950 doc: Switch boost::thread to std::thread in scheduler (MarcoFalke) fa98196 test: Remove unused scheduler.h include from the common setup (MarcoFalke) fa609c4 scheduler: Remove unused REVERSE_LOCK (MarcoFalke) Pull request description: This accumulates a bunch of cleanup that was long overdue, but I haven't yet gotten around to address. Specifically, but not limited to: * Remove unused code, documentation and includes * Upgrade to doxygen documentation Please refer to the individual commits for more details. ACKs for top commit: jnewbery: utACK fa8337f Tree-SHA512: 0c825ad9767e2697a3ef1ec1be13fdc2b18eeb7493ad0be5b65cc9f209391e78b17ee66e35e094c5e171c12b0f1624f287a110f6bddaf3024b708877afa8552e

…e function declarations and calls cc29d1e [tools] Update clang-format config (John Newbery) Pull request description: In some cases, running clang-format has made code _less_ readable by joining declarations and calls for functions with many arguments into very long lines. For example: ``` - size_t getQueueInfo(std::chrono::system_clock::time_point &first, - std::chrono::system_clock::time_point &last) const; + size_t getQueueInfo(std::chrono::system_clock::time_point& first, std::chrono::system_clock::time_point& last) const; ``` (bitcoin#19090 (comment)) This change to clang-format would allow arguments/parameters for func declarations/calls to be split over multiple lines, aligned with the opening parens. It does not force args/params to be on new lines (that setting is `BinPackParameters : true`). ACKs for top commit: MarcoFalke: ACK cc29d1e fine with me practicalswift: ACK cc29d1e Tree-SHA512: a62474925e71aaff41bdce7960fd5ffd64317da810f694d8084080b054708cf71c2ab2ce3111db5a9260d1c1f9e02d59a2ecb5543b1b6172ce085cb42432160a

Summary: > No longer needed after [[bitcoin/bitcoin#17270 | core#17270]] This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [1/6] bitcoin/bitcoin@fa609c4 Backport note: the original PR has 7 commits, but the last one is not relevant to our codebase. Test Plan: With tsan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9955

Summary: The common setup is included in virtually all tests, so it should be as slim as possible. This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [2/7] bitcoin/bitcoin@fa98196 Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9956

Summary: > After [[bitcoin/bitcoin#18234 | core#18234]] the scheduler itself no longer cares if the > serviceQueue is run in a std::thread or boost::thread. Change the > documentation to std::thread because we switched to C++11. This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [3/6] bitcoin/bitcoin@fab2950 Test Plan: Proofreading Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9957

Summary: This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [4/6] bitcoin/bitcoin@fa9cca0 Test Plan: NA Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9958

Summary: This helps understanding the code at the call site without having to look up the name of the argument or the default value. This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [5/6] bitcoin/bitcoin@fac43f9 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9959

Summary: This is a backport of [[bitcoin/bitcoin#19090 | core#19090]] [6/6] bitcoin/bitcoin@fa3d41b Test Plan: Proofreading Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9960

backports bitcoin/bitcoin@fab2950 doc: Switch boost::thread to std::thread in scheduler After commit d0ebd93 the scheduler itself no longer cares if the serviceQueue is run in a std::thread or boost::thread. Change the documentation to std::thread because we switched to C++11.

MarcoFalke added 2 commits May 28, 2020 09:00

scheduler: Remove unused REVERSE_LOCK

fa609c4

No longer needed after commit d61f2bb

test: Remove unused scheduler.h include from the common setup

fa98196

The common setup is included in virtually all tests, so it should be as slim as possible.

maflcko added the Refactoring label May 28, 2020

ajtowns reviewed May 28, 2020

View reviewed changes

src/scheduler.h Outdated Show resolved Hide resolved

src/scheduler.h Outdated Show resolved Hide resolved

MarcoFalke added 2 commits May 28, 2020 10:22

doc: Switch boost::thread to std::thread in scheduler

fab2950

After commit d0ebd93 the scheduler itself no longer cares if the serviceQueue is run in a std::thread or boost::thread. Change the documentation to std::thread because we switched to C++11.

doc: Remove unused documentation about unimplemented features

fa9cca0

maflcko force-pushed the 2005-schedulerCleanup branch from fa5645b to fae1ef5 Compare May 28, 2020 14:38

jnewbery reviewed May 28, 2020

View reviewed changes

src/scheduler.h Outdated Show resolved Hide resolved

src/scheduler.h Outdated Show resolved Hide resolved

src/scheduler.h Outdated Show resolved Hide resolved

jnewbery mentioned this pull request May 28, 2020

[tools] Update clang-format config for multi-line function declarations and calls #19095

Merged

maflcko force-pushed the 2005-schedulerCleanup branch from fae1ef5 to fa9bdf9 Compare May 28, 2020 18:23

MarcoFalke added 2 commits May 28, 2020 19:28

scheduler: Replace stop(true) with StopWhenDrained()

fac43f9

This helps understanding the code at the call site without having to look up the name of the argument or the default value.

doc: Switch scheduler to doxygen comments

fa3d41b

maflcko force-pushed the 2005-schedulerCleanup branch from fa9bdf9 to fa197f1 Compare May 28, 2020 23:29

clang-format scheduler

fa8337f

Can easily be reviewed with the git options --ignore-all-space --word-diff-regex=. -U0

maflcko force-pushed the 2005-schedulerCleanup branch from fa197f1 to fa8337f Compare June 21, 2020 10:03

maflcko merged commit 3a4a372 into bitcoin:master Jun 25, 2020

maflcko deleted the 2005-schedulerCleanup branch June 25, 2020 14:38

This was referenced Sep 9, 2021

Boost usage reduction DeFiCh/ain#714

Closed

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

str4d mentioned this pull request Dec 6, 2021

Backport upstream PRs that remove Boost usage zcash/zcash#4819

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Misc scheduler cleanups#19090

refactor: Misc scheduler cleanups#19090
maflcko merged 7 commits intobitcoin:masterfrom
maflcko:2005-schedulerCleanup

maflcko commented May 28, 2020

Uh oh!

practicalswift commented May 28, 2020

Uh oh!

ajtowns left a comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented May 28, 2020

Uh oh!

maflcko commented May 28, 2020

Uh oh!

jnewbery commented May 29, 2020

Uh oh!

maflcko commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

maflcko commented May 28, 2020

Uh oh!

practicalswift commented May 28, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented May 28, 2020

Uh oh!

maflcko commented May 28, 2020

Uh oh!

jnewbery commented May 29, 2020

Uh oh!

maflcko commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants