test: Replace gArgs with local argsman in bench #18662
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maflcko
force-pushed
the
2004-toolsArgsman
branch
from
fad6b6d to
facf901
Compare
added 3 commits
April 15, 2020 20:22
-BEGIN VERIFY SCRIPT- sed -i -e 's/gArgs/argsman/g' src/bench/bench_bitcoin.cpp -END VERIFY SCRIPT-
maflcko
force-pushed
the
2004-toolsArgsman
branch
from
facf901 to
fae00a7
Compare
maflcko
changed the title
promag
reviewed
Apr 16, 2020
Contributor
|
Concept ACK On behalf of future generations on Bitcoin Core developers: thanks for doing these cleanups - that will make their lives easier :) The short-term gain from each individual clean-up might seem small but the cumulative long-term gain will make a big difference. |
Contributor
|
ACK faf989f. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 17, 2020
faf989f util: Document why ArgsManager (con/de)structor is not inline (MarcoFalke) fae00a7 bench: Remove unused argsman.ClearArgs (MarcoFalke) fa46aeb scripted-diff: Replace gArgs with local argsman in bench (MarcoFalke) fa2bc41 tools: Add unused argsman to bench_bitcoin (MarcoFalke) Pull request description: All utilities use the same gArgs global that the node uses. This is generally fine and does not lead to issues unless a bench test is going to spin up a NodeContext via the TestingSetup. In that case the two uses of gArgs conflict and currently it needs to be cleared: https://github.com/bitcoin/bitcoin/blob/544709763e1f45148d1926831e07ff03487673ee/src/bench/bench_bitcoin.cpp#L76 One solution would be to do nothing, because the current code works with that workaround. Another solution would be to not use the same global in all binaries. ACKs for top commit: promag: ACK faf989f. ryanofsky: Code review ACK faf989f. Just new commit added restoring forward declaration Tree-SHA512: 8ee4b28eee294d41c002f801fa844b0c23c919a3061f5109638701db0947b3b0ea28caa7311ae5f126fc660648bbaa0890853e6b06bdc5868692f52ba8c05f66
maflcko
pushed a commit
that referenced
this pull request
Jul 30, 2020
… args 8ed9002 refactor: use local argsmanager in CRegTestParams (Ivan Metlushko) 9b20f66 scripted-diff: Replace gArgs with local argsman (Ivan Metlushko) a316e9c refactor: add unused ArgsManager to replace gArgs (Ivan Metlushko) Pull request description: Rationale: reduce use of gArgs to decouple code and simplify future maintenance and easier unit testing. This PR is continuation of work started in #18926 and #18662 It covers only places that register args in ArgsManager with `AddArgs()` or `AddHiddenArgs()`. Closes #19511 ACKs for top commit: MarcoFalke: ACK 8ed9002 👛 Tree-SHA512: 7e6ba8e8357a48833c71e9c3942a769acb3d93bdcc6748a8ef2b7c4461a2499419b60896abf1d8b6bf8e88ee2590284cdd5da64220243ac22375300bcb8fe3e8
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 23, 2020
Summary: * tools: Add unused argsman to bench_bitcoin * scripted-diff: Replace gArgs with local argsman in bench -BEGIN VERIFY SCRIPT- sed -i -e 's/gArgs/argsman/g' src/bench/bench_bitcoin.cpp -END VERIFY SCRIPT- * bench: Remove unused argsman.ClearArgs * util: Document why ArgsManager (con/de)structor is not inline Backport of Core [[bitcoin/bitcoin#18662 | PR18662]] Test Plan: ninja bench-bitcoin Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8079
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All utilities use the same gArgs global that the node uses. This is generally fine and does not lead to issues unless a bench test is going to spin up a NodeContext via the TestingSetup. In that case the two uses of gArgs conflict and currently it needs to be cleared:
bitcoin/src/bench/bench_bitcoin.cpp
Line 76 in 5447097
One solution would be to do nothing, because the current code works with that workaround. Another solution would be to not use the same global in all binaries.