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 https://github.com/jtimon/bitcoin/compare/0.13-args-class...jtimon:0.13-args-remove-wrapers for the less disruptive ones to remove.
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” notifications@github.com 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 https://github.com/jtimon/bitcoin/compare/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:
– 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: https://github.com/bitcoin/bitcoin/pull/9494
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).
379 }
380
381-bool IsArgSet(const std::string& strArg)
382+const std::vector<std::string>& ArgsManager::ArgsAt(const std::string& strArg) const
383+{
384+ return mapMultiArgs.at(strArg);
Concept ACK, will review.
On March 24, 2017 2:57:16 PM PDT, Pieter Wuille notifications@github.com wrote:
utACK c67209128a114e3260ab93c3e8a63b1b23663b34
– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9494#issuecomment-289152860
92@@ -93,7 +93,7 @@ static bool multiUserAuthorized(std::string strUserPass)
93 std::string strUser = strUserPass.substr(0, strUserPass.find(":"));
94 std::string strPass = strUserPass.substr(strUserPass.find(":") + 1);
95
96- if (mapMultiArgs.count("-rpcauth") > 0) {
97+ if (argsGlobal.IsArgSet("-rpcauth") > 0) {
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?
414+ mapMultiArgs[str].push_back(strValue);
415 }
416 }
417
418-bool IsArgSet(const std::string& strArg)
419+std::vector<std::string> ArgsManager::ArgsAt(const std::string& strArg)
GetArgs seems like a better name?
481 LOCK(cs_args);
482 mapArgs[strArg] = strValue;
483+ mapMultiArgs[strArg].push_back(strValue);
484 }
485
486+void ParseParameters(int argc, const char* const argv[])
233@@ -226,6 +234,20 @@ bool SoftSetBoolArg(const std::string& strArg, bool fValue);
234
235 // Forces a arg setting, used only in testing
236 void ForceSetArg(const std::string& strArg, const std::string& strValue);
237+};
238+
239+extern ArgsManager argsGlobal;
Args? :)
ArgsManager& args parameters instead of using the global. Or do you mean s/ArgsManager/Args/ ? I’m fine with that.
Also, maybe s/argsGlobal/gArgs/ too, but I don’t like extern ArgsManager args
extern ArgsManager Args, but gArgs is probably good enough.
733@@ -734,7 +734,7 @@ void InitParameterInteraction()
734 LogPrintf("%s: parameter interaction: -whitebind set -> setting -listen=1\n", __func__);
735 }
736
737- if (mapMultiArgs.count("-connect") && mapMultiArgs.at("-connect").size() > 0) {
738+ if (argsGlobal.IsArgSet("-connect") && argsGlobal.ArgsAt("-connect").size() > 0) {
IsArgSet be true, and ArgsAt().size() be zero?
903@@ -904,9 +904,9 @@ bool AppInitParameterInteraction()
904 InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
905
906 // ********************************************************* Step 3: parameter-to-internal-flags
907- if (mapMultiArgs.count("-debug") > 0) {
908+ if (argsGlobal.IsArgSet("-debug")) {
909 // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
910- const std::vector<std::string>& categories = mapMultiArgs.at("-debug");
911+ const std::vector<std::string>& categories = argsGlobal.ArgsAt("-debug");
You can. The lifetime of the temporary is extended by taking a reference to it. This code is effectively equivalent to:
0const std::vector<std::string> categories_tmp = argsGlobal.ArgsAt("-debug");
1const std::vector<std::string>& categories = categories_tmp;
Renamed s/globalArgs/gArgs/ as suggested discussed with @luke-jr here. @TheBlueMatt asked for using scripted-diff so I ended up “bikeshedding the history” quite a bit, but I think it’s clearer now. I tried to follow @luke-jr ’s suggestion #9494 (review) in more places, but I don’t understand why this breaks the python tests yet: https://github.com/jtimon/bitcoin/commit/32bffa0162a0ebd66c554dab4b35705e664845dc
Also, sorry for going back and forth with removing some wrappers directly or not, but with scripted-diff I think it’s better to just remove them all at once after this. But if we want to do the simpler ones without scripted-diff here I still have that in https://github.com/jtimon/bitcoin/tree/b15-args-class-wrappers
481-void ForceSetArg(const std::string& strArg, const std::string& strValue)
482+void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue)
483 {
484 LOCK(cs_args);
485 mapArgs[strArg] = strValue;
486+ _mapMultiArgs[strArg].push_back(strValue);
242@@ -235,6 +243,55 @@ bool SoftSetBoolArg(const std::string& strArg, bool fValue);
243
244 // Forces a arg setting, used only in testing
245 void ForceSetArg(const std::string& strArg, const std::string& strValue);
246+};
“Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs”
Indentation for all the methods above is missing. Though it’s understandable not to add it now to make diff smaller.
910@@ -911,9 +911,9 @@ bool AppInitParameterInteraction()
911 InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
912
913 // ********************************************************* Step 3: parameter-to-internal-flags
914- if (gArgs.IsArgSet("-debug") > 0) {
915+ if (gArgs.IsArgSet("-debug")) {
916 // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
917- const std::vector<std::string>& categories = gArgs.GetArgs("-debug");
918+ const std::vector<std::string> categories = gArgs.GetArgs("-debug");
Because #9494 (review) and I don’t care either way for the reasons @sipa explained.
EDIT: yet I’m again happy to turn it back.
- Introduce ArgsManager::GetArgs()
- Adapt util_tests.cpp to ArgsManager
-BEGIN VERIFY SCRIPT-
sed -i 's/mapMultiArgs.count(/gArgs.IsArgSet(/g' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
sed -i 's/mapMultiArgs.at("/gArgs.GetArgs("/g' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
-END VERIFY SCRIPT-
- Set ArgsManager::mapMultiArgs in ArgsManager::SoftSetArg, ForceSetArg, SoftSetBoolArg
- Don't check gArgs.IsArgSet() is greater than 0
- Remove unneeded calls and local variables