refactor: Consistently use context args over gArgs in node/interfaces #27239

1. 
   maflcko commented at 11:33 AM on March 10, 2023: member

   Currently node/interfaces.cpp uses a mix of gArgs vs m_context->args. This is fine, because outside of tests those should be identical. However, it makes the code inconsistent and harder to use in tests.

   Fix that by using args from the context consistently. Do the same in init.cpp, where gArgs and args are inconsistently used in the same scope or even line.

2. 
   DrahtBot commented at 11:33 AM on March 10, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky
   Concept ACK	fanquake
   Stale ACK	TheCharlatan

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
   • #25268 (refactor: Introduce EvictionManager by dergoegge)
   • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
   • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
   • #10102 (Multiprocess bitcoin by ryanofsky)

   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.

3. DrahtBot renamed this:
   refactor: Consistenly use context args over gArgs in node/interfaces
   refactor: Consistenly use context args over gArgs in node/interfaces
   on Mar 10, 2023
4. DrahtBot added the label Refactoring on Mar 10, 2023
5. TheCharlatan approved
6. 
   TheCharlatan commented at 12:52 PM on March 10, 2023: contributor

   ACK facbb44

   This has been confusing me recently, thank you for cleaning this up. I reproduced the patchset and did not find similar mixed uses of context and gArgs in other files.

7. 
   fanquake commented at 2:30 PM on March 10, 2023: member

   Concept ACK

8. in src/node/interfaces.cpp:391 in 11110f2e3d outdated

    387 | @@ -388,6 +388,7 @@ class NodeImpl : public Node
    388 |      {
    389 |          m_context = context;
    390 |      }
    391 | +    ArgsManager& argsman() { return *Assert(Assert(m_context)->args); }
   

   ---

   ---

   

   ryanofsky commented at 3:57 PM on March 10, 2023:

   In commit "refactor: Consistenly use context args over gArgs in node/interfaces" (11110f2e3d97d7d88ada1fdaab70a866bf9a35d7)

   Slight preference for calling this args not argsman. args is used more commonly in code (921 vs 340 times before this commit). Also I always thought the name ArgsManager was a misnomer, because it's really just a data container that doesn't manage any significant internal state.

   ---

   

   maflcko commented at 4:31 PM on March 10, 2023:

   thx, done

9. ryanofsky approved
10. 
    ryanofsky commented at 4:00 PM on March 10, 2023: contributor

    Code review ACK facbb444bf5aea2bbaa4a4246a8a2c661d9bf314. Thanks for the cleanup. Could s/consistenly/consistently/ too

11. maflcko renamed this:
    refactor: Consistenly use context args over gArgs in node/interfaces
    refactor: Consistently use context args over gArgs in node/interfaces
    on Mar 10, 2023
12. refactor: Consistently use context args over gArgs in node/interfaces fa891120c8
13. refactor: Consistently use args over gArgs in init.cpp fa3e9b420f
14. maflcko force-pushed on Mar 10, 2023
15. ryanofsky approved
16. 
    ryanofsky commented at 4:35 PM on March 10, 2023: contributor

    Code review ACK fa3e9b420fbc1ce9b20218f03356502e1b79d5ff

17. DrahtBot requested review from TheCharlatan on Mar 10, 2023
18. 
    TheCharlatan commented at 4:53 PM on March 10, 2023: contributor

    re-ACK facbb44

19. 
    fanquake commented at 10:19 AM on March 11, 2023: member

    @TheCharlatan note that you've re-ACK'd the wrong commit hash.

20. fanquake merged this on Mar 11, 2023
21. fanquake closed this on Mar 11, 2023

    ---

22. maflcko deleted the branch on Mar 11, 2023
23. bitcoin deleted a comment on Mar 12, 2023
24. sidhujag referenced this in commit 682de9c68a on Mar 12, 2023
25. bitcoin locked this on Mar 11, 2024

Contributors

maflckoDrahtBotTheCharlatanfanquakeryanofsky

Review Requested
TheCharlatan