Util: Remove ArgsManager wrappers: #10119

pull jtimon wants to merge 6 commits into bitcoin:master from jtimon:0.14-args-wrappers changing 17 files +168 −114

jtimon commented at 7:22 AM on March 30, 2017: contributor
- ParseParameters
- ReadConfigFile
- ForceSetArg
- SoftSetArg
Includes:
- Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494
From the wrappers newly introduced with ArgsManager, these seem currently the easiest to remove.
fanquake added the label Refactoring on Mar 30, 2017
in src/util.cpp:417 in 11b82c4e32 outdated
```
 378 | +        mapMultiArgs[str].push_back(strValue);
 379 |      }
 380 |  }
 381 |  
 382 | -bool IsArgSet(const std::string& strArg)
 383 | +std::vector<std::string> ArgsManager::ArgsAt(const std::string& strArg)
```
JeremyRubin commented at 5:50 PM on March 30, 2017:

Any reason to not return a const ref?

Also, ArgsAt is a bit of an odd name, perhaps ArgsFor or something (but maybe not worth the effort to redo this)

jtimon commented at 8:07 PM on April 5, 2017:

See #9494 (review) for some reason all your reviews are appearing in #10119 instead of #9494 . Can you make the comments there for other reviewers to get them? Remember after #9494 this PR (#10119) is a single commit (that could be squashed to #9494 or not)

JeremyRubin commented at 8:26 PM on April 5, 2017:

oh -- I didn't understand 'includes' to mean builds on the PR. gotcha

in src/util.cpp:474 in 11b82c4e32 outdated

 442 | -void ForceSetArg(const std::string& strArg, const std::string& strValue)
 443 | +void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue)
 444 |  {
 445 |      LOCK(cs_args);
 446 |      mapArgs[strArg] = strValue;
 447 | +    mapMultiArgs[strArg].push_back(strValue);

JeremyRubin commented at 5:55 PM on March 30, 2017:

Should this push_back or replace?

jtimon commented at 1:20 PM on April 4, 2017:

I believe push_back, but it's a good point, I am not sure now.

in src/util.cpp:477 in 11b82c4e32 outdated
```
 445 |      LOCK(cs_args);
 446 |      mapArgs[strArg] = strValue;
 447 | +    mapMultiArgs[strArg].push_back(strValue);
 448 |  }
 449 |  
 450 | +bool IsArgSet(const std::string& strArg)
```
JeremyRubin commented at 5:56 PM on March 30, 2017:

Is the PR too big if you get rid of these?

jtimon commented at 3:17 PM on April 4, 2017:

The less disruptive to remove are removed in #10119 I'm happy to squash that in #9494 Others are more disruptive to remove, but I'm working on doing it for SoftSetBoolArg and IsArgSet in https://github.com/jtimon/bitcoin/compare/0.14-args-wrappers...jtimon:0.14-args-isargset-wrapper but I'm not sure everybody will like that way of doing it.
in src/util.h:177 in 11b82c4e32 outdated
```
 130 | @@ -133,6 +131,16 @@ inline bool IsSwitchChar(char c)
 131 |  #endif
 132 |  }
 133 |  
 134 | +class ArgsManager
 135 | +{
 136 | +protected:
 137 | +    CCriticalSection cs_args;
 138 | +    std::map<std::string, std::string> mapArgs;
 139 | +    std::map<std::string, std::vector<std::string> > mapMultiArgs;
```
JeremyRubin commented at 5:57 PM on March 30, 2017:

why not use multimap?

jtimon commented at 1:30 PM on April 4, 2017:

I was just using the same that was used before. We can move to multimap, what would be the gain?

JeremyRubin commented at 2:10 PM on April 4, 2017:

No real gain, just slightly simpler (because multi_map is intended to be used I think exactly in this type of case).

jtimon commented at 8:09 PM on April 5, 2017:

I thought it was more for when you wanted the elements on the vector to be ordered in certain way or something. Leaving the same structure seems easier to review, but once encapsulated it is easy to make further improvements to ArgsManager's internal implementation.
JeremyRubin commented at 6:14 PM on March 30, 2017: contributor

utack
jtimon force-pushed on Apr 4, 2017
jtimon commented at 3:17 PM on April 4, 2017: contributor

Needed rebase.
jtimon force-pushed on Apr 5, 2017
Util: Create ArgsManager class 8623e251b6
Util: Set _mapMultiArgs in ArgsManager::SoftSetArg, ForceSetArg, SoftSetBoolArg b9e117b1fa
Util: s/mapMultiArgs.count(/argsGlobal.IsArgSet(/ 2aa8d32585
Util: Introduce ArgsManager::ArgsAt() ce8a5d6f1a
Util: Put mapMultiArgs inside ArgsManager 996d2ae6ac

Util: Remove ArgsManager wrappers:

- ParseParameters
- ReadConfigFile
- ForceSetArg
- SoftSetArg

9d6c531320

jtimon force-pushed on Apr 6, 2017
jtimon commented at 1:11 PM on May 5, 2017: contributor

Incorportated in #9494
jtimon closed this on May 5, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

jtimon

JeremyRubin

Labels

Refactoring

Linked (view graph)

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