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).
concept ACK :+1: , will review again when able, once-over utACK 8a96939
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);
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.
Needed rebase.
Needed rebase (due to "Refactor: Remove using namespace <xxx>" PRs).
Concept ACK, seems a move in the right direction.
utACK c67209128a114e3260ab93c3e8a63b1b23663b34
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) {
nit: kill the > 0 here?
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).
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?
I'd prefer to just fix the issue - I think its super strange that SoftSetArg, etc dont keep mapMultiArgs and mapArgs consistent.
Fixed nits. Also removed a bunch of BOOST_FOREACH for free in the commit that introduces ArgsManager::ArgsAt().
Fixed nit.
Needed rebase.
put my review of this code on the wrong PR. #10119 (review)
Needed rebase again.
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[])
Suggest moving these to the header as static inline.
I suspect a performance improvement is expected by doing so. Thanks
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;
Can we just call it Args? :)
I was thinking that maybe some functions could get a local 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
I meant 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) {
Under what conditions would IsArgSet be true, and ArgsAt().size() be zero?
After the last change suggested by @TheBlueMatt this should never be possible, good catch.
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");
Can you reference a temporary like this?
You can. The lifetime of the temporary is extended by taking a reference to it. This code is effectively equivalent to:
const std::vector<std::string> categories_tmp = argsGlobal.ArgsAt("-debug");
const std::vector<std::string>& categories = categories_tmp;
At the same time, I didn't do it on purpose here. I'm happy to remove the "&" if that's preferred.
Looks basically okay.
Fixed some nits. I made the wrappers static inline and removed the less disruptive ones (which include ParseParameters and ReadConfigFile) as I was doing in #10119 (just squashed it to the first commit here, plus adapted 1 new use of SoftSetArg after rebase), but since that's a little bit more disruptive I didn't made the methods ParseParameters and ReadConfigFile inline yet. Also found more cases where ArgsAt/GetArgs didn't need to be called after calling IsArgSet.
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);
Its somewhat annoying to have an intermediate commit which breaks our locking in the middle - maybe move this one line to the "Put mapMultiArgs inside ArgsManager" commit? It should at least break fewer things by doing so.
utACK f19f5b5dbd0e3b6f73ded3818014eed3f728e5fc
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.
Yes, that's exactly the point, but I have done similar things in the past and not fixed them later, something I feel bad about, for example: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1313 Please, help me make sure that it gets done this time.
utACK f19f5b5dbd0e3b6f73ded3818014eed3f728e5fc
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");
why?
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
utACK f19f5b5dbd0e3b6f73ded3818014eed3f728e5fc
Sorry, just pushed again changing the history as @TheBlueMatt suggested. 78da882 should be equivalent to f19f5b5
utACK 78da882edd54ed62f4a0035590460a97cb9ff282. Confirmed no diff since previous review, just mapMultiArgs commit history change.
utACK 78da882