Skip to content

Pass argsmanager reference to intro instead of relying on global#75

Closed
maflcko wants to merge 1 commit intobitcoin-core:masterfrom
maflcko:2008-qtArgsIntro
Closed

Pass argsmanager reference to intro instead of relying on global#75
maflcko wants to merge 1 commit intobitcoin-core:masterfrom
maflcko:2008-qtArgsIntro

Conversation

@maflcko
Copy link
Contributor

@maflcko maflcko commented Aug 26, 2020 •
edited
Loading

Motivation: (copied from bitcoin/bitcoin#19779)

The gArgs global has several issues:

  • gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value.
  • Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. Net processing: move ProcessMessage() to PeerLogicValidation bitcoin/bitcoin#19704 (comment) or #19511

The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs to use a passed-in reference instead.

@maflcko
Copy link
Contributor Author

maflcko commented Aug 26, 2020

requested by @ryanofsky in #35 (comment)

@maflcko maflcko changed the title qt: Pass argsmanager reference to intro instead of relying on global Pass argsmanager reference to intro instead of relying on global Aug 26, 2020
@hebasto
Copy link
Member

hebasto commented Aug 28, 2020

Concept ACK.

hebasto
hebasto approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa1b478, tested on Linux Mint 20 (x86_64) with the -choosedatadir command-line option.

@maflcko
Copy link
Contributor Author

maflcko commented Aug 31, 2020

@ryanofsky Would be good to have your eyes over this as a sanity check 👀

@maflcko maflcko closed this Aug 31, 2020
@maflcko maflcko reopened this Aug 31, 2020
@ryanofsky
Copy link
Contributor

ryanofsky commented Sep 9, 2020

I don't think this change is beneficial in it's current form. Before this change there are 46 references to the gArgs global in src/qt/, and after this change there are 40 references to gArgs. I think if you really want to get rid of the gArgs global in the GUI you could do it in a single PR instead of having gArgs and args variables used different places unpredictably.

Also I don't think there is a necessarily a reason to eliminate the gArgs in the GUI. It's understandable to want to get rid of global variables in node and wallet code to allow more code reuse and help testing, but the GUI already relies on a global QApplication instance, a global event loop, global settings state and probably other global state, so getting rid of gArgs there doesn't seem to accomplish much. I think removing gArgs from node and wallet code and then making gArgs a GUI-only variable would satisfy goals stated in the PR description while keeping GUI code less verbose, and requiring fewer changes.

@maflcko
Copy link
Contributor Author

maflcko commented Sep 9, 2020

Ok, thanks for looking.

@maflcko maflcko closed this Sep 9, 2020
@maflcko maflcko deleted the 2008-qtArgsIntro branch September 9, 2020 18:58
@maflcko maflcko mentioned this pull request Jan 25, 2021
Closed
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maflcko @hebasto @ryanofsky