-
Notifications
You must be signed in to change notification settings - Fork 38.7k
refactor: Stop using gArgs global in system.cpp #27170
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
Conversation
New function was introduced by willcl-ark <[email protected]> in commit 56e370f from bitcoin#27073 and removes some duplicate code.
Most of the code in util/system.cpp that was hardcoded to use the global ArgsManager instance `gArgs` has been changed to work with explicit ArgsManager instances (for example in bitcoin#20092). But a few hardcoded references to `gArgs` remain. This commit removes the last ones so these functions aren't reading or writing global state.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Concept ACK |
|
Concept ACK |
|
Concept ACK. |
stickies-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
light code review ACK 9a9d5da
willcl-ark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 9a9d5da
Tidy changes to remove the last references to global gArgs from util/system.cpp (besides the global declaration).
stickies-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9a9d5da
| } | ||
|
|
||
| // If datadir is changed in .conf file: | ||
| gArgs.ClearPathCache(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Looks like this shouldn't have been called on gArgs regardless of this PR, perhaps useful to split out in separate commit? In the 3 callsites of ArgsManager::ReadConfigFiles I see, it is only called on gArgs anyway, so I don't see any actual behaviour change/potential bug because of it.
|
ACK 9a9d5da |
Most of the code in
util/system.cppthat was hardcoded to use the globalArgsManagerinstancegArgshas been changed to stop using it (for example in #20092). But a few hardcoded references togArgsremain. This commit removes the last ones so these functions aren't reading or writing global state.Noticed these
gArgsreferences while reviewing #27073