Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494

jtimon · 2017-01-09T16:02:00Z

Continues #9243 by @TheBlueMatt
Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs.

To reduce disruption, the used functions are kept as wrapers. The wrapers can be removed later, see jtimon/bitcoin@0.13-args-class...jtimon:0.13-args-remove-wrapers for the less disruptive ones to remove.

TheBlueMatt · 2017-01-09T20:35:45Z

Concept ACK. I think we had discussed doing this when #9243 went in.

…

On January 9, 2017 11:02:04 AM EST, "Jorge Timón" ***@***.***> wrote: Continues #9243 by @TheBlueMatt Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs. To reduce disruption, the used functions are kept as wrapers. The wrapers can be removed later, see jtimon@0.13-args-class...jtimon:0.13-args-remove-wrapers for the less disruptive ones to remove. You can view, comment on, or merge this pull request online at: #9494 -- Commit Summary -- * Util: Create ArgsManager class * Util: Expose ArgsManager argsGlobal * Util: s/mapMultiArgs.count(/argsGlobal.IsArgSet(/ * Util: Introduce ArgsManager::ArgsAt() * Util: Put mapMultiArgs inside ArgsManager -- File Changes -- M src/httprpc.cpp (4) M src/httpserver.cpp (8) M src/init.cpp (48) M src/net.cpp (10) M src/test/util_tests.cpp (77) M src/util.cpp (83) M src/util.h (28) -- Patch Links -- https://github.com/bitcoin/bitcoin/pull/9494.patch https://github.com/bitcoin/bitcoin/pull/9494.diff -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #9494

jtimon · 2017-01-09T22:47:06Z

Yes, we discussed it in #9243, but that this seems approximately what was discussed previously is still very useful, thanks.

What concerns me more of the current code is the fact that the methods IsArgSet, GetArg, GetBoolArg and ArgsAt aren't const, because they need to LOCK(cs_args);. The first "solution" that comes to mind is to make a superclass with just these methods (but they remain non-const).

dcousens · 2017-01-11T06:50:47Z

concept ACK 👍 , will review again when able, once-over utACK 8a96939

jtimon · 2017-01-11T13:34:20Z

src/util.cpp

-bool IsArgSet(const std::string& strArg)
+const std::vector<std::string>& ArgsManager::ArgsAt(const std::string& strArg) const
+{
+    return mapMultiArgs.at(strArg);


I assume this should get LOCK(cs_args); too? If so, the method can't be const.

Yes, it also cant return a reference, needs to make a copy.

jtimon · 2017-01-31T21:38:34Z

Needed rebase.

jtimon · 2017-03-23T22:14:20Z

Needed rebase (due to "Refactor: Remove using namespace " PRs).

laanwj · 2017-03-24T12:55:51Z

Concept ACK, seems a move in the right direction.

sipa · 2017-03-24T21:57:12Z

utACK c67209128a114e3260ab93c3e8a63b1b23663b34

TheBlueMatt · 2017-03-25T01:52:47Z

Concept ACK, will review.

…

On March 24, 2017 2:57:16 PM PDT, Pieter Wuille ***@***.***> wrote: utACK c67209128a114e3260ab93c3e8a63b1b23663b34 -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #9494 (comment)

TheBlueMatt

Switching mapMultiArgs.count() to argsGlobal.IsArgSet makes a few things incorrect, because the SoftSetArgs stuff doesn't update mapMultiArgs but does update mapArgs. I think the correct solution here is to keep mapMultiArgs and mapArgs consistent (@luke-jr also needed that for the multiwallet pull), though that does mean ArgsAt needs to return a copy and not a reference (which I think is OK - arg usage doesn't need to be fast).

TheBlueMatt · 2017-03-28T14:01:48Z

src/httprpc.cpp

nit: kill the > 0 here?

jtimon · 2017-03-28T18:46:48Z

Switching mapMultiArgs.count() to argsGlobal.IsArgSet makes a few things incorrect, because the SoftSetArgs stuff doesn't update mapMultiArgs but does update mapArgs.

Maybe a solution could be to use argsGlobal.IsArgsAt().size() > 0 in those cases?
I mean, I think in all those cases will just try to loop through an empty vector (ie do nothing) inside the if clause, but I haven't made sure of that. I could go through them and maybe replace some BOOST_FOREACH for C++ fors while at it.
Never mind, it seems mapMultiArgs.count("-bind") is not like that, but could be fixed with argsGlobal.IsArgsAt().size() > 0
By "making mapMultiArgs and mapArg" I assume you just mean redundantly setting to mapMultiArgs in ArgsManager::SoftSetArg too as it is done in ArgsManager::ArgsAt. Perhaps that is simpler and easier to reason about.

Thoughts?

TheBlueMatt · 2017-03-28T19:08:48Z

I'd prefer to just fix the issue - I think its super strange that SoftSetArg, etc dont keep mapMultiArgs and mapArgs consistent.

jtimon · 2017-03-29T14:57:51Z

Fixed nits. Also removed a bunch of BOOST_FOREACH for free in the commit that introduces ArgsManager::ArgsAt().

jtimon · 2017-03-29T21:25:12Z

Fixed nit.

jtimon · 2017-04-04T15:10:51Z

Needed rebase.

JeremyRubin · 2017-04-05T20:28:13Z

put my review of this code on the wrong PR. #10119 (comment)

theuni · 2017-05-09T19:39:41Z

utACK f19f5b5

jtimon · 2017-05-09T19:41:26Z

Sorry, just pushed again changing the history as @TheBlueMatt suggested.
78da882 should be equivalent to f19f5b5

ryanofsky · 2017-05-10T15:41:08Z

utACK 78da882. Confirmed no diff since previous review, just mapMultiArgs commit history change.

laanwj · 2017-05-15T05:39:21Z

utACK 78da882

…pArgs and mapMultiArgs 78da882 Util: Small improvements in gArgs usage (Jorge Timón) 5292245 Util: Put mapMultiArgs inside ArgsManager (Jorge Timón) b3cbd55 scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs (Jorge Timón) f2957ce Util: Create ArgsManager class... (Jorge Timón) Tree-SHA512: 7d58250da440ad0f41745f46ab6021d6ecbb292035cab3d86fb08ce6bd822df604ac31b3ded6fd6914f7cfd12ba531cbc06a76eb500f629627f47ae6ac8350a7

…rgs, mapArgs and mapMultiArgs 78da882 Util: Small improvements in gArgs usage (Jorge Timón) 5292245 Util: Put mapMultiArgs inside ArgsManager (Jorge Timón) b3cbd55 scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs (Jorge Timón) f2957ce Util: Create ArgsManager class... (Jorge Timón) Tree-SHA512: 7d58250da440ad0f41745f46ab6021d6ecbb292035cab3d86fb08ce6bd822df604ac31b3ded6fd6914f7cfd12ba531cbc06a76eb500f629627f47ae6ac8350a7 add ForceRemoveArg and ForceSetMultiArg to ArgsManager class Signed-off-by: Pasta <[email protected]> add static inlines for ForceSetMultiArgs and ForceRemoveArg Signed-off-by: Pasta <[email protected]> both void Signed-off-by: Pasta <[email protected]> use gArgs, dash code Signed-off-by: Pasta <[email protected]> revert a bit Signed-off-by: Pasta <[email protected]> adj Signed-off-by: Pasta <[email protected]> use gArgs Signed-off-by: Pasta <[email protected]> remove '_' Signed-off-by: Pasta <[email protected]>

4709ff8 remove now unused gArgs wrappers (Fuzzbawls) cc40f8d Manual prep for gArg wrapper removal (Fuzzbawls) b3c843c scripted-diff: stop using the gArgs wrappers (Fuzzbawls) 5ce3844 Migrate g_logger fields to gArgs (Fuzzbawls) 72a6f56 Track negated arguments in the argument paser. (Fuzzbawls) 3142e1b Add additional tests for GetBoolArg() (Fuzzbawls) 4aef2f3 Fix constness of ArgsManager methods (Fuzzbawls) 13fd27d Fix indentation after 'Remove redundant calls to gArgs.IsArgSet()' (Fuzzbawls) a4114f3 Util: Remove redundant calls to gArgs.IsArgSet() (Fuzzbawls) 92e7227 Util: Small improvements in gArgs usage (Fuzzbawls) a8b26e7 Util: Put mapMultiArgs inside ArgsManager (Fuzzbawls) d2f2e25 scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs (Fuzzbawls) b2abc59 Util: Create ArgsManager class... (Fuzzbawls) 8c4d9c5 Add a ForceSetArg method for testing (Fuzzbawls) 59d6529 Lock mapArgs/mapMultiArgs access in util (Fuzzbawls) f7f2689 Un-expose mapArgs from util.h (Fuzzbawls) 7894b07 Get rid of mapArgs direct access in ZMQ construction (Matt Corallo) b28789f Introduce (and use) an IsArgSet accessor method (Fuzzbawls) 4d8ecf9 Fix non-const mapMultiArgs[] access after init. (Fuzzbawls) 391388d Remove arguments to ReadConfigFile (Fuzzbawls) Pull request description: This is a cumulation of several upstream backports surrounding the argument parsing code. Included here are the following upstream PRs: - bitcoin#9243 - bitcoin#9494 - bitcoin#10118 - bitcoin#10901 - bitcoin#12713 - bitcoin#10607 I've also refactored the global logging argument parsing to match, as that was an "out-of-order" PR that was previously backported to match our previous state. ACKs for top commit: random-zebra: utACK 4709ff8 furszy: cool encapsulation, utACK 4709ff8 and merging Tree-SHA512: 74c8dc1b2280b726eff44855e9908f3563d4ef6493f296ebfafe240ac6470bc45cbc75baac614ed59dfda9e1377b7da7d47a695dfc05e364cada2468bb99a258

jtimon force-pushed the 0.13-args-class branch from ff58b28 to 8a96939 Compare January 9, 2017 16:21

jtimon mentioned this pull request Jan 9, 2017

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

fanquake added the Refactoring label Jan 9, 2017

dcousens approved these changes Jan 11, 2017

View reviewed changes

jtimon commented Jan 11, 2017

View reviewed changes

jtimon force-pushed the 0.13-args-class branch from 8a96939 to 6086a84 Compare January 31, 2017 21:37

jtimon force-pushed the 0.13-args-class branch from 6086a84 to 94fa561 Compare January 31, 2017 23:39

jtimon force-pushed the 0.13-args-class branch from 94fa561 to c672091 Compare March 23, 2017 22:13

TheBlueMatt reviewed Mar 28, 2017

View reviewed changes

src/httprpc.cpp Outdated

Copy link

Contributor

TheBlueMatt Mar 28, 2017

Choose a reason for hiding this comment

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

nit: kill the > 0 here?

jtimon reacted with thumbs up emoji

TheBlueMatt mentioned this pull request Mar 28, 2017

Basic multiwallet support #8694

Merged

jtimon force-pushed the 0.13-args-class branch from c672091 to 09ba58d Compare March 29, 2017 14:56

jtimon force-pushed the 0.13-args-class branch 2 times, most recently from 4396063 to 5c18d51 Compare March 29, 2017 21:24

This was referenced Mar 29, 2017

Util: Remove redundant calls to argsGlobal.IsArgSet() #10118

Merged

Util: Remove ArgsManager wrappers: #10119

Closed

jtimon force-pushed the 0.13-args-class branch from 5c18d51 to a3a7220 Compare April 4, 2017 15:07

laanwj merged commit 78da882 into bitcoin:master May 15, 2017

jtimon deleted the 0.13-args-class branch May 18, 2017 20:25

jnewbery mentioned this pull request May 30, 2017

[trivial] Fix comment for ForceSetArg() #10479

Merged

jnewbery mentioned this pull request Jun 14, 2017

[trivial] fix indentation for ArgsManager class #10592

Merged

benma mentioned this pull request Jun 15, 2017

scripted-diff: stop using the gArgs wrappers #10607

Closed

Fuzzbawls mentioned this pull request Oct 23, 2020

[Core] Arg parsing overhaul PIVX-Project/PIVX#1938

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494

Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494

Uh oh!

Conversation

jtimon commented Jan 9, 2017

Uh oh!

TheBlueMatt commented Jan 9, 2017 via email

Uh oh!

jtimon commented Jan 9, 2017

Uh oh!

dcousens commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 31, 2017

Uh oh!

jtimon commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 24, 2017

Uh oh!

sipa commented Mar 24, 2017

Uh oh!

TheBlueMatt commented Mar 25, 2017 via email

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 28, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 28, 2017

Uh oh!

TheBlueMatt commented Mar 28, 2017

Uh oh!

jtimon commented Mar 29, 2017

Uh oh!

jtimon commented Mar 29, 2017

Uh oh!

jtimon commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Apr 5, 2017

Uh oh!

theuni commented May 9, 2017

Uh oh!

jtimon commented May 9, 2017

Uh oh!

ryanofsky commented May 10, 2017

Uh oh!

laanwj commented May 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dcousens commented Jan 11, 2017 •

edited

Loading

jtimon commented Mar 23, 2017 •

edited

Loading

jtimon commented Apr 4, 2017 •

edited

Loading