IsArgSet is problematic:
IsArgSet is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.Get*Arg calls can already return an std::optional nullopt value to indicate an unset arg.So relieve both issues by removing all IsArgSet that are redundant.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31896.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc, ryanofsky |
| Concept ACK | yancyribbens, jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
maxfeerate wallet startup option by ismaelsadeeq)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.
It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback.
I’m not 100% sure when reading the commit what is meant by dead code. Is it that the arg -blockmintxfee is a no longer used switch here? I ask because I notice some other calls to GetArg with fallbacks such as:
0src/init.cpp:879: if (args.IsArgSet("-upnp")) {
1011@@ -1012,20 +1012,20 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1012
1013 // Sanity check argument for min fee for including tx in block
1014 // TODO: Harmonize which arguments need sanity checking and where that happens
It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback.
I’m not 100% sure when reading the commit what is meant by dead code.
It is just saying the fallback argument is unused. For example in:
0 if (gArgs.IsArgSet("-color")) {
1 {
2 const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
3 ...
4 }
The DEFAULT_COLOR_SETTING argument is unused, and any fallback value that is passed would be ignored because IsArgSet returned true.
3122- std::optional<CAmount> min_tx_fee = ParseMoney(args.GetArg("-mintxfee", ""));
3123+ if (const auto arg{args.GetArg("-mintxfee")}) {
3124+ std::optional<CAmount> min_tx_fee = ParseMoney(*arg);
3125 if (!min_tx_fee) {
3126- error = AmountErrMsg("mintxfee", args.GetArg("-mintxfee", ""));
3127+ error = AmountErrMsg("mintxfee", *arg);
args.GetArg were removed…
cr & tACK fa3fb3c23fae287b161112b9b04cf0937a1051c6
What about the IsArgSet ones followed by GetPathArg?
What about the
IsArgSetones followed byGetPathArg?
Not sure what you mean, but GetPathArg does not return std::optional<_>, so I don’t think the changes that are done here are applicable to it. There may or may not be cleanups related to GetPathArg, but my recommendation would be to create a separate, unrelated and independent issue or pull request to discuss or propse changes.
re-ACK fa0cf040baa74b161b4b00ddea95bfb5aac0bb53
Since my last review: outdated TODO comment was removed (on new commit) - no impact.
1077@@ -1078,7 +1078,7 @@ static void ParseGetInfoResult(UniValue& result)
1078 }
1079 #endif
1080
1081- if (gArgs.IsArgSet("-color")) {
1082+ {
1156@@ -1157,11 +1157,10 @@ bool CheckHostPortOptions(const ArgsManager& args) {
1157 "-port",
1158 "-rpcport",
1159 }) {
1160- if (args.IsArgSet(port_option)) {
1161- const std::string port = args.GetArg(port_option, "");
1162+ if (const auto port{args.GetArg(port_option)}) {
fa3fb3c23fae287b161112b9b04cf0937a1051c6 For all GetArg calls in this commit, perhaps I am confused but should they be const ref instead? Would also be clearer to continue to specify the type here, as before on line 1161, to avoid having to look up the type returned by GetArg().
0 if (const std::string& port{args.GetArg(port_option)}) {
The getters return a value, not a pointer or reference. C++ allows to bind the return value to either a reference or a value, but the end result is the same and either is fine here in this context.
However, the type is not std::string as claimed by you. (This can be seen by trying to compile your suggestion with a C++ compiler and observing the compile error).
Also, the type wasn’t mentioned previously in the code for most of the GetArg calls, so it seems better to leave it this way.
1009@@ -1010,8 +1010,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1010 return InitError(Untranslated("peertimeout must be a positive integer."));
1011 }
1012
1013- // Sanity check argument for min fee for including tx in block
1014- // TODO: Harmonize which arguments need sanity checking and where that happens
Concept ACK.
Part of the commit message of fa41685f438bba00761423d464bbb8dc5ea7ddf6 seems incorrect (maybe was pasted from fa3fb3c23fae287b161112b9b04cf0937a1051c6 without updating, not sure).
Checking for IsArgSet before calling GetArg while providing the args
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
However, ignoring the fallback value is fragile, because it would not be
sanitized.
Fix all issues by sanitizing the fallback value.
Checking for IsArgSet before calling GetArg while providing an arbitrary
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
Even better would be to provide the true fallback value and sanitize it
as if it were user-input, but this can be done in a follow-up.
Removing the redundant call to IsArgSet will have to be done either way,
so do it now.
If anything is left to be done, a new discussion issue or pull request
can be created.
re-ACK 0000fb3fd9f01f8a8cbe9de0bdc080023b6f00fc
No changes, just rebased (basically due to this #31278).