Refactor: Add Flags enum to ArgsManager class

hebasto commented at 9:15 pm on May 26, 2019: member

This PR adds the Flags enum to the ArgsManager class. Also the m_flags member is added to the Arg struct. Flags denote an allowed type of an arg value and special hints.

This PR is only a refactoring and does not change behavior.

fanquake added the label Refactoring on May 26, 2019

fanquake added the label Utils/log/libs on May 26, 2019

fanquake requested review from ryanofsky on May 26, 2019

hebasto renamed this:
~~Prevent meaningless negating of arguments~~
WIP: Prevent meaningless negating of arguments
on May 26, 2019

DrahtBot commented at 11:01 pm on May 26, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16527 (B: Get rid of Params().RequireStandard() by jtimon)
#16526 (A: Refactor: Chainparams: readability by jtimon)
#16524 (Wallet: Disable -fallbackfee by default by jtimon)
#16411 (Signet support by kallewoof)
#16115 (On bitcoind startup, write config args to debug.log by LarryRuane)
#16060 (Bury bip9 deployments by jnewbery)
#15934 (Separate settings merging from parsing by ryanofsky)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15421 (torcontrol: Launch a private Tor instance when not already running by luke-jr)
#15367 (feature: Added ability for users to add a startup command by benthecarman)
#14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
#13716 (bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords by kallewoof)
#13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)
#12557 ([WIP] 64 bit iOS device support by Sjors)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
#8994 (Testchains: Introduce custom chain whose constructor… by jtimon)

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.

in src/util/system.cpp:341 in 9ad3972652 outdated

317@@ -319,6 +318,27 @@ ArgsManager::ArgsManager() :
318       "-port", "-bind",
319       "-rpcport", "-rpcbind",
320       "-wallet",
321+    },
322+
323+    m_allowed_negated_args{

ryanofsky commented at 12:31 pm on May 28, 2019:

The problem described in the PR is real but I don’t think the change implemented is the simplest or best fix. Having a new whitelist of options located in a separate part of the code from where the options are used and defined adds complexity, inconsistency, and action at distance to already very confusing code.

If someone specifies -nopid, it is wrong to create a pid file called 0, but a simpler and more useful fix would be to be not write a pid file instead of adding a new logic in a different part of the code to print a “negating not allowed” error.

But the -pid argument case actually seems atypical here. More cases where negated arguments are handled incorrectly (like -incrementalrelayfee) seem like simple misuses of the IsArgSet function, which is misnamed and doesn’t do what people think it does. I’d replace the IsArgSet function with separate ArgHasValue and ArgIsSpecified functions and use ArgHasValue instead of ArgIsSpecified everywhere or almost everywhere the code currently using ArgIsSet.

hebasto commented at 7:55 pm on July 16, 2019:

m_allowed_negated_args is used in the same manner as m_network_only_args (see #11862) and it seems consistent, IMO.

promag commented at 1:32 pm on July 23, 2019:

adds complexity, inconsistency, and action at distance to already very confusing code

Agree with @ryanofsky. I’d rather see an approach like #16416 (comment)

Beside the fact that m_network_only_args is very small, I think it could be improved like my comment above - move that flag to the Arg structure.

Currently ArgsManager::Arg has:

0        bool m_debug_only;

And I think it could be changed to something like:

0   enum Flags {
1      NONE = 0x00,
2      DEBUG_ONLY = 0x01,
3      NETWORK_ONLY = 0x2,
4      ALLOW_NEGATED = 0x04,      
5   };
6
7   Flags m_flags;

Or just add more members.

hebasto force-pushed on Jul 16, 2019

hebasto renamed this:
~~WIP: Prevent meaningless negating of arguments~~
Prevent meaningless negating of arguments
on Jul 16, 2019

hebasto commented at 8:04 pm on July 16, 2019: member

@ryanofsky Thank you for your review.

If someone specifies -nopid, it is wrong to create a pid file called 0, but a simpler and more useful fix would be to be not write a pid file instead of adding a new logic in a different part of the code to print a “negating not allowed” error. But the -pid argument case actually seems atypical here.

Agree that -nopid should have a special behavior. And it deserves a separate PR.

More cases where negated arguments are handled incorrectly (like -incrementalrelayfee) seem like simple misuses of the IsArgSet function, which is misnamed and doesn’t do what people think it does. I’d replace the IsArgSet function with separate ArgHasValue and ArgIsSpecified functions and use ArgHasValue instead of ArgIsSpecified everywhere or almost everywhere the code currently using ArgIsSet.

I don’t think this approach will compeltely fix the problem described in this PR.

hebasto force-pushed on Jul 23, 2019

hebasto commented at 11:48 pm on July 23, 2019: member

@promag

And I think it could be changed to something like:

0   enum Flags {
1      NONE = 0x00,
2      DEBUG_ONLY = 0x01,
3      NETWORK_ONLY = 0x2,
4      ALLOW_NEGATED = 0x04,      
5   };
6
7   Flags m_flags;

Done.

hebasto commented at 5:30 am on July 24, 2019: member

Commit 50a7f97db2a9daa1ed4f32e64cfb87b02e205e70 needs rebase on top of #16366. Going to make it tonight.

ryanofsky commented at 5:39 am on July 24, 2019: member

Thanks for the update! I still see several problems with this approach, but I think they can be fixed and I have specific suggestions below that I think will put this PR in a good state and make it easier to review. Here are the problems that I see presently (90dfd237ea75301891fc1715ebf319966f2e0b86):

Interaction between boolean option detection via help string and ALLOW_NEGATED errors via flags is confusing and I think will lead to bugs in the future.
PR has no release notes, no clear description of changes in behavior understandable to a normal user.
PR mixes refactoring changes with behavioral changes, and makes behavior changes in commits that affects swaths of options rather than individual options, so it is difficult as a reviewer to determine how behavior is changing and verify the changes are good without searching and looking at every option.

I think all of these problems are fixable. Here are my suggestions:

Drop the detection of boolean arguments through help strings and drop the ALLOW_NEGATED flag. The only flags I would suggest keeping here are:

0ALLOW_BOOL = 0x01,
1ALLOW_INT = 0x02,
2ALLOW_STRING = 0x4,
3ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
4DEBUG_ONLY = 0x100,
5NETWORK_ONLY = 0x200,

Do not change any behavior in this PR. Set every existing option to ALLOW_ANY by default, with | DEBUG_ONLY and | NETWORK_ONLY flags added as necessary to preserve behavior.
Raise an error during startup if a -noXXX option is given and the ALLOW_BOOL flag is not present. This should have no effect on behavior of existing options because ALLOW_ANY should be set as described above.
Optionally, if you feel like implementing extra error checking that could be useful in the future, check for properly formatted ints and bools and nonempty strings based on the int/bool/string flags. Again these checks should not change behavior of existing options because ALLOW_ANY should always be set here.
In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.
Make separate PRs for help text cleanups, any other unrelated cleanups.

Overall, I think this is moving in a good direction and adding the flags really helps. But the more we can move in the direction of simple bool/int/string option types, and have negation taken care of on the front end, rather than leaking into entire codebase, the better state this code will be in, and less weird behavior we will have in the future.

hebasto commented at 10:03 am on July 24, 2019: member

@ryanofsky Thank you for your review. I really appreciate it.

In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

We have over two hundred target lines of code. Proposed approach does not allow to apply a scripted-diff, unfortunately. Thoughts?

hebasto commented at 10:48 am on July 24, 2019: member

@ryanofsky

Drop the detection of boolean arguments through help strings …

Agree. Going to implement it.

… and drop the ALLOW_NEGATED flag.

I’d prefer to use it. It complements the IsArgNegated() function. Also, consider the following two options:

-nowallet has a special meaning, -wallet=0 points to a wallet named 0 and -nowallet=0 is an error; possible flags are ALLOW_BOOL | ALLOW_STRING
-noprune equals to -prune=0; -noprune=0 is not an error; possible flags are ALLOW_BOOL | ALLOW_INT

Using the ALLOW_NEGATED flag makes things simpler, IMO:

-wallet has flags ALLOW_STRING | ALLOW_NEGATED
-prune has flags ALLOW_INT | ALLOW_NEGATED

ryanofsky commented at 1:17 pm on July 24, 2019: member

In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

We have over two hundred target lines of code. Proposed approach does not allow to apply a scripted-diff, unfortunately. Thoughts?

That’s the whole point! It’s good to use scripted diffs in refactoring commits, but there should be no scripted diff for the behavior change. The code changes in the behavior change PR should just look like:

0-gArgs.AddArg("-changetype=<n>", ALLOW_ANY, ...);
1+gArgs.AddArg("-changetype=<n>", ALLOW_STRING, ...);

0-gArgs.AddArg("-dbcache=<n>", ALLOW_ANY, ...);
1+gArgs.AddArg("-dbcache=<n>", ALLOW_INT, ...);

So it is easy for reviewers to verify that new errors triggered on options that currently allow negation are appropriate and appropriately documented in release notes.

There should not be 200 lines targeted. Every existing boolean option, every existing list option, most string options, and many int options that accept 0 should continue to allow negation.

… and drop the ALLOW_NEGATED flag.

I’d prefer to use it. It complements the IsArgNegated() function.

Just like IsArgSet which is misused many places, misunderstood and should be eliminated. IsArgNegated is an awkward API that should go away, and ALLOW_NEGATED should never be introduced.

If you look at google flags api, qsettings, python argparse, the standard everywhere is bool/number/string representations, not “is negated” queries for code reading the values. My refactoring PR #15934 is a major step in this direction internally replacing our vector representation of negated values with plain, typed false values.

Nothing I’m suggesting here involves changing or limiting behavior in any way. I’m just suggesting to use normal, sane terminology, so we have a more usable api and so the IsArgSet IsArgNegated bugs you are fixing now don’t get reintroduced in the future.

Using the ALLOW_NEGATED flag makes things simpler, IMO:

-wallet has flags ALLOW_STRING | ALLOW_NEGATED

-prune has flags ALLOW_INT | ALLOW_NEGATED

That’s exactly what I’m suggesting, except spelling ALLOW_NEGATED as ALLOW_BOOL.

I didn’t fully describe in my previous comment how the suggested flags should behave, so I’ll do that now to avoid unnecessary confusion. I don’t think the extra error checks I’m describing here need to be implemented now. It’d be fine to implement them in future PRs that actually start adding ALLOW_BOOL / ALLOW_INT / ALLOW_STRING options instead of ALLOW_ANY for everything. Anyway, here’s how the flags are meant to behave:

-noOPTION and -noOPTION=1 negation syntax should be accepted when ALLOW_BOOL is present.
-OPTION and -OPTION= empty string syntax be accepted when ALLOW_BOOL is present.
-OPTION=0 and -OPTION=1 syntax should be accepted when ALLOW_BOOL is present.
-OPTION=123 where 123 is an integer should be accepted when ALLOW_INT is present.
-OPTION=abc where abc is a nonempty string should be accepted when ALLOW_STRING is present.
Anything not accepted above should trigger an error.

Except for one detail, adding these checks should have no effect when ALLOW_ANY is specified, so even if these checks are added now, this can remain a pure refactoring PR and not change behavior.

The detail is that we currently accept -noOPTION=abc syntax for strange values of abc, sometimes interpreting abc as false and showing a warning, sometimes interpreting it as true and not warning. This behavior should be left untouched here, and cleaned up in a separate PR (making any abc that is not the empty string or 1 an error).

promag commented at 1:25 pm on July 24, 2019: member

Really nice @ryanofsky. Maybe it could also have a MULTIPLE flag?

ryanofsky commented at 1:54 pm on July 24, 2019: member

Maybe it could also have a MULTIPLE flag?

Yes I think that’d be a good flag, and I also like your idea adding support to declare default values. Best if this PR scope doesn’t expand too much, but going forward, the more things about options that can be declared up front, the less complicated code reading options has to be and the less chance for inconsistencies and bugs.

promag commented at 2:11 pm on July 24, 2019: member

Best if this PR scope doesn’t expand too much

👍

Another flag (otherwise I may forget it) could be SENSITIVE so that its value wouldn’t be dumped.

hebasto commented at 8:01 pm on July 26, 2019: member

@ryanofsky

6. Make separate PRs for help text cleanups, any other unrelated cleanups.

The “refactoring: Use direct list initialization” commit (b89eb79b3a7bc3c2a0cd80454f4373f7a07f2fd5) has split out to #16469.

ryanofsky commented at 8:22 pm on July 26, 2019: member

@ryanofsky
06. Make separate PRs for help text cleanups, any other unrelated cleanups.
The “refactoring: Use direct list initialization” commit (b89eb79) has split out to #16469.

Sorry, I’d suggest closing #16469. The quote above is a little ambiguous, but I was trying to suggest splitting behavior changes into a separate PRs from the requested flags refactor, not creating multiple refactoring PRs.

It’s good to make behavior changes in small independent PRs so they don’t get missed by reviewers and so there aren’t multiple confusing discussion about different behaviors. It generally takes a lot less mental effort to review a pure refactoring PR than it does to review a refactoring PR mixed with behavior changes, so there shouldn’t be any problem with including a small list initialization refactor in the same PR that does the requested flag refactor.

MarcoFalke commented at 8:32 pm on July 26, 2019: member

Agree with @ryanofsky

refactoring: Check IsArgKnown() early e0e18a1017

Refactor InterpretNegatedOption() function

- added args parameter
- renamed to InterpretOption()
- removed code duplication

e0d187dfeb

Add Flags enum to ArgsManager 265c1b58d8

scripted-diff: Use Flags enum in AddArg()

-BEGIN VERIFY SCRIPT-
sed -i 's/const bool debug_only,/unsigned int flags, &/' src/util/system.h src/util/system.cpp
sed -i -E 's/(true|false), OptionsCategory::/ArgsManager::ALLOW_ANY, &/' $(git grep --files-with-matches 'AddArg(' src)
-END VERIFY SCRIPT-

1b4b9422ca

scripted-diff: Use ArgsManager::DEBUG_ONLY flag

-BEGIN VERIFY SCRIPT-
sed -i 's/unsigned int flags, const bool debug_only,/unsigned int flags,/' src/util/system.h src/util/system.cpp
sed -i 's/ArgsManager::NONE, debug_only/flags, false/' src/util/system.cpp
sed -i 's/arg.second.m_debug_only/(arg.second.m_flags \& ArgsManager::DEBUG_ONLY)/' src/util/system.cpp
sed -i 's/ArgsManager::ALLOW_ANY, true, OptionsCategory::/ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::/' $(git grep --files-with-matches 'AddArg(' src)
sed -i 's/ArgsManager::ALLOW_ANY, false, OptionsCategory::/ArgsManager::ALLOW_ANY, OptionsCategory::/' $(git grep --files-with-matches 'AddArg(' src)
-END VERIFY SCRIPT-

fb4b9f9e3b

Remove unused m_debug_only member from Arg struct 9a12733508

hebasto force-pushed on Jul 27, 2019

Use ArgsManager::NETWORK_ONLY flag dde80c272a

Replace IsArgKnown() with FlagsOfKnownArg() db08edb303

hebasto force-pushed on Jul 27, 2019

hebasto commented at 8:35 pm on July 27, 2019: member

@ryanofsky

Do not change any behavior in this PR. Set every existing option to ALLOW_ANY by default, with | DEBUG_ONLY and | NETWORK_ONLY flags added as necessary to preserve behavior.

Raise an error during startup if a -noXXX option is given and the ALLOW_BOOL flag is not present. This should have no effect on behavior of existing options because ALLOW_ANY should be set as described above.

Done.

In a followup PR, replace ALLOW_ANY with ALLOW_STRING or ALLOW_INT for selected options where the goal is to disallow negation. This PR should be small, trivial to review, and have release notes describing the user visible changes and options that are affected.

ALLOW_ANY is replaced with ALLOW_STRING in #16476.

hebasto renamed this:
~~Prevent meaningless negating of arguments~~
Add Flags enum to ArgsManager class
on Jul 27, 2019

hebasto commented at 8:45 pm on July 27, 2019: member

PR title and description have been updated.

in test/functional/feature_config_args.py:81 in 3f0b5e28eb outdated

77@@ -78,7 +78,7 @@ def test_config_file_parser(self):
78             conf.write('')  # clear
79 
80     def test_log_buffer(self):
81-        with self.nodes[0].assert_debug_log(expected_msgs=['Warning: parsed potentially confusing double-negative -connect=0']):
82+        with self.nodes[0].assert_debug_log(expected_msgs=['Warning: parsed potentially confusing double-negative -noconnect=0']):

ryanofsky commented at 4:48 pm on July 30, 2019:

In commit “Revamp option negating policy” (3f0b5e28ebcf42c168f393a3109403354cb8e128)

This commit appears to be changing behavior by fixing the error message so it technically contradicts “This PR is only a refactoring and does not change behavior” in the PR description. Could consider removing this here and just doing it as a minor bugfix later. Or updating the PR description to mention the change in behavior.

hebasto commented at 3:19 pm on July 31, 2019:

Could consider removing this here and just doing it as a minor bugfix later.

Done. Moved to #16508.

ryanofsky commented at 4:52 pm on July 30, 2019: member

This looks great! I need to check this more throughly to give a review ACK, but I went through all the commits, and they are all simple, well structured cleanups that should make this easy to merge.

hebasto force-pushed on Jul 31, 2019

hebasto renamed this:
~~Add Flags enum to ArgsManager class~~
Refactor: Add Flags enum to ArgsManager class
on Jul 31, 2019

Revamp option negating policy b70cc5d733

test: Make tests arg type specific e6f649cb2c

hebasto force-pushed on Jul 31, 2019

in src/util/system.cpp:300 in e0d187dfeb outdated

292@@ -294,17 +293,18 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
293         ++option_index;
294     }
295     if (key.substr(option_index, 2) == "no") {
296-        bool bool_val = InterpretBool(val);
297+        const bool bool_val = InterpretBool(val);
298         key.erase(option_index, 2);
299         if (!bool_val ) {
300             // Double negatives like -nofoo=0 are supported (but discouraged)
301             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);

jamesob commented at 4:08 pm on August 1, 2019:

(https://github.com/bitcoin/bitcoin/pull/16097/commits/e0d187dfeb18b026de22bd7960b2a50c2b958e1a)

FWIW, this results in a slightly incorrect error message:

0$ ./src/bitcoind -datadir=/data/bitcoin -noconnect=0 -logthreadnames
1
22019-08-01T16:03:23Z Warning: parsed potentially confusing double-negative -connect=0

which I think should read “double-negative -noconnect=0”.

hebasto commented at 4:33 pm on August 1, 2019:

I think should read “double-negative -noconnect=0”.

You are right. This remained untouched to preserve the old behavior and make this PR refactoring only. Details: #16097 (review) and #16097 (review).

jamesob approved

jamesob commented at 4:13 pm on August 1, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/16097/commits/e6f649cb2c07bf55d9214c2876619c56f1d6fe30

Commit checklist

e0e18a1017 refactoring: Check IsArgKnown() early

Checks whether an argument is recognized before determining whether it’s a negation.
e0d187dfeb Refactor InterpretNegatedOption() function

Changes the way negated options are processed (DRYs up code, modifies args map inline).
265c1b58d8 Add Flags enum to ArgsManager

Defines the various flags available to ArgsManager.
1b4b9422ca scripted-diff: Use Flags enum in AddArg()

Straightforward scripted-diff adding flags argument to AddArg(), filling with degenerate ALLOW_ANY default.
fb4b9f9e3b scripted-diff: Use ArgsManager::DEBUG_ONLY flag

Collapses the AddArg debug_only parameter into flags. Cumbersome to review even for a scripted-diff because of the remarkable line length and number of changes but… it’s a scripted-diff on arg processing code ¯_(ツ)_/¯.
9a12733508 Remove unused m_debug_only member from Arg struct
dde80c272a Use ArgsManager::NETWORK_ONLY flag

Manually specifies NETWORK_ONLY flag for certain arguments that were previously special-cased away from their AddArg definitions.
db08edb303 Replace IsArgKnown() with FlagsOfKnownArg()
b70cc5d733 Revamp option negating policy
e6f649cb2c test: Make tests arg type specific

Tests

Built and ran ./src/test/test_bitcoin successfully.

Not sure if this was a preexisting issue, but it’s kind of weird that when I double-negate connect, we seem to get socket errors.

 0./src/bitcoind -datadir=/data/bitcoin -noconnect=0 -logthreadnames
 1
 22019-08-01T16:03:23Z Warning: parsed potentially confusing double-negative -connect=0
 3
 4...
 5
 62019-08-01T16:03:40Z [opencon] opencon thread start
 72019-08-01T16:03:40Z [msghand] msghand thread start
 82019-08-01T16:03:40Z [opencon] socket send error Broken pipe (32)
 92019-08-01T16:03:40Z [opencon] socket send error Broken pipe (32)
102019-08-01T16:03:41Z [opencon] socket send error Broken pipe (32)
112019-08-01T16:03:43Z [opencon] socket send error Broken pipe (32)
122019-08-01T16:03:45Z [opencon] socket send error Broken pipe (32)
132019-08-01T16:03:47Z [opencon] socket send error Broken pipe (32)
142019-08-01T16:03:50Z [opencon] socket send error Broken pipe (32)
152019-08-01T16:03:54Z [opencon] socket send error Broken pipe (32)

I’m not really sure how to test the NETWORK_ONLY stuff (which doesn’t seem to be covered by unittests), but this works:

0$ ./src/bitcoind -datadir=/data/bitcoin -testnet -port=4444 -nologthreadnames=0
1
2[as expected]

ryanofsky commented at 3:02 pm on August 2, 2019: member

I’m glad this PR isn’t trying to fully implement this flags yet, since it keeps the change light and simple, and because the flag implementations will make more sense in context of followup PRs which actually put them to use. But I think I would like to see simple asserts added here to prevent misapplication of the new flags by developers, and bugs in the future when the flags actually do start being enforced:

 0diff --git a/src/util/system.cpp b/src/util/system.cpp
 1index 5f704ecabef..8ad02128c57 100644
 2--- a/src/util/system.cpp
 3+++ b/src/util/system.cpp
 4@@ -461,6 +461,7 @@ unsigned int ArgsManager::FlagsOfKnownArg(const std::string& key) const
 5 
 6 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 7 {
 8+    assert(FlagsOfKnownArg(strArg) & ALLOW_STRING);
 9     std::vector<std::string> result = {};
10     if (IsArgNegated(strArg)) return result; // special case
11 
12@@ -504,6 +505,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
13 
14 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
15 {
16+    assert(FlagsOfKnownArg(strArg) & ALLOW_STRING);
17     if (IsArgNegated(strArg)) return "0";
18     std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
19     if (found_res.first) return found_res.second;
20@@ -512,6 +514,7 @@ std::string ArgsManager::GetArg(const std::string& strArg, const std::string& st
21 
22 int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const
23 {
24+    assert(FlagsOfKnownArg(strArg) & ALLOW_INT);
25     if (IsArgNegated(strArg)) return 0;
26     std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
27     if (found_res.first) return atoi64(found_res.second);
28@@ -520,6 +523,7 @@ int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const
29 
30 bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
31 {
32+    assert(FlagsOfKnownArg(strArg) & ALLOW_BOOL);
33     if (IsArgNegated(strArg)) return false;
34     std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
35     if (found_res.first) return InterpretBool(found_res.second);

in src/util/system.h:131 in e6f649cb2c

126@@ -127,16 +127,31 @@ struct SectionInfo
127 
128 class ArgsManager
129 {
130+public:
131+    enum Flags {

MarcoFalke commented at 3:51 pm on August 2, 2019:

style-nit (for follow-up). Could indicate that this derives from unsigned, not int (which is the default)

in src/util/system.h:154 in e6f649cb2c

152         std::string m_help_param;
153         std::string m_help_text;
154-        bool m_debug_only;
155-
156-        Arg(const std::string& help_param, const std::string& help_text, bool debug_only) : m_help_param(help_param), m_help_text(help_text), m_debug_only(debug_only) {};
157+        unsigned int m_flags;

MarcoFalke commented at 3:58 pm on August 2, 2019:

style-nit (for follow-up) could make const, to enforce the constructor sets it and it is never modified afterward.

MarcoFalke approved

MarcoFalke commented at 4:17 pm on August 2, 2019: member

ACK e6f649cb2c07bf55d9214c2876619c56f1d6fe30 thanks for adding types to the command line options

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e6f649cb2c07bf55d9214c2876619c56f1d6fe30 thanks for adding types to the command line options
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUixQAv+PcIolVlZDyU/bWh0rKnyH4rRnFKm/ZrjcLrXWFby/fC5x8spb/1Kizcx
 8TtMKnLEP3YXZTo0PgOyqIemGsVit+6NkIVvpIbPjz/M6VTz1CLLZp0rdRWAHxUKs
 9hX3Un0yAch+IK/IEq3J813TOSCFlHWkdy+5LNzC77sGx5M4nJk302QmsRvlAgEsc
10IlJAfSqEk1wF6MaQ9yhdIrnfOBiiM/ZnXCOQK6VAO4uw2xUrl1Cd3f9L8lO8CxWm
11O+3vy51F84af2oDSpKuhmT4Vcyl+/H25UNb3pNWRky+KmmGYSGKXSuWWOz8EK9/3
12KHq6pEajoRWDG6qsZnUyzS4QQAx9Vf2ZTsBh37UmRj3AgS6Ct/jKsYJvL69QP+C4
139zPIs4NQNP24dkGTNx7R7TcoUU0P60O72Mop8aqEhwnQPaffyXlWTuRs8sjYdsBL
1462JcofhWZRG++V8l9bafcyBKCn6sIV0C6ztAgDgvR0gW7wrX0xOdckUyrRecDncP
15U47QO/n4
16=pQlB
17-----END PGP SIGNATURE-----

Timestamp of file with hash 60ba83da959664c8ccc4f7e6dac1c5da28003ce06779868d2def12da094a0be4 -

MarcoFalke referenced this in commit 3a3d8b8357 on Aug 2, 2019

MarcoFalke merged this on Aug 2, 2019

MarcoFalke closed this on Aug 2, 2019

ryanofsky commented at 4:48 pm on August 2, 2019: member

utACK e6f649cb2c07bf55d9214c2876619c56f1d6fe30 (post-merge)

hebasto deleted the branch on Aug 2, 2019

ryanofsky referenced this in commit 2b97772c6e on Nov 14, 2019

ryanofsky referenced this in commit bcf8060557 on Nov 14, 2019

ryanofsky referenced this in commit cba2710220 on Dec 18, 2019

ryanofsky referenced this in commit 67518f7cc6 on Dec 19, 2019

ryanofsky referenced this in commit 27beca5ffe on Jan 7, 2020

ryanofsky referenced this in commit 5bb512aa51 on Jan 7, 2020

ryanofsky referenced this in commit f42da6bbe3 on Jan 24, 2020

HashUnlimited referenced this in commit 51d492e9c1 on Apr 17, 2020

deadalnix referenced this in commit 80898a1ef3 on Apr 17, 2020

deadalnix referenced this in commit 43e52180c7 on Apr 17, 2020

deadalnix referenced this in commit d5106f943c on Apr 17, 2020

deadalnix referenced this in commit fd105ab49f on Apr 20, 2020

deadalnix referenced this in commit 10ace580f5 on Apr 20, 2020

deadalnix referenced this in commit c940708cea on Apr 20, 2020

deadalnix referenced this in commit a03db8775a on Apr 20, 2020

deadalnix referenced this in commit 4dd8ff7d41 on Apr 20, 2020

deadalnix referenced this in commit 61d9fe4bc3 on Apr 20, 2020

deadalnix referenced this in commit d487542960 on Apr 20, 2020

ftrader referenced this in commit cdabaddb86 on Aug 17, 2020

ryanofsky referenced this in commit 850019babd on Sep 2, 2020

ryanofsky referenced this in commit 662bf36367 on Sep 2, 2020

ryanofsky referenced this in commit a7a1518176 on Sep 2, 2020

ryanofsky referenced this in commit 9cde44dc32 on Sep 2, 2020

ryanofsky referenced this in commit f5282e5e1b on Sep 2, 2020

ryanofsky referenced this in commit f801ea91aa on Sep 2, 2020

ryanofsky referenced this in commit 45beb3bc1a on Sep 2, 2020

ryanofsky referenced this in commit c213464e72 on Mar 22, 2021

ryanofsky referenced this in commit b8ed0b221f on Mar 22, 2021

ryanofsky referenced this in commit 64f92731b5 on Mar 24, 2021

backpacker69 referenced this in commit ba6f9db598 on Mar 28, 2021

ryanofsky referenced this in commit 5ef6e720e0 on Mar 29, 2021

ryanofsky referenced this in commit 3f3fcdd5f9 on Mar 29, 2021

ryanofsky referenced this in commit e8a460024d on Mar 29, 2021

ryanofsky referenced this in commit dcd1d3c536 on Jun 2, 2021

ryanofsky referenced this in commit 2772948473 on Jun 2, 2021

ryanofsky referenced this in commit 13e43176c4 on Aug 19, 2021

ryanofsky referenced this in commit 0a4ded27b7 on Aug 19, 2021

ryanofsky referenced this in commit aa3dec078f on Aug 19, 2021

ryanofsky referenced this in commit 9349a02cc4 on Aug 23, 2021

ryanofsky referenced this in commit 553b2dfb85 on Aug 23, 2021

ryanofsky referenced this in commit c7ba71fb91 on Aug 26, 2021

ryanofsky referenced this in commit a56d2a2ef0 on Aug 26, 2021

vijaydasmp referenced this in commit 2850d22d61 on Oct 29, 2021

vijaydasmp referenced this in commit 245c036330 on Oct 29, 2021

vijaydasmp referenced this in commit 212689254e on Oct 29, 2021

vijaydasmp referenced this in commit f1d527d22a on Oct 30, 2021

ryanofsky referenced this in commit 4eec25dc73 on Nov 1, 2021

ryanofsky referenced this in commit 3345e119b0 on Nov 1, 2021

vijaydasmp referenced this in commit 0281922100 on Nov 2, 2021

kittywhiskers referenced this in commit 3b02336024 on Nov 6, 2021

kittywhiskers referenced this in commit e790676a63 on Nov 6, 2021

kittywhiskers referenced this in commit 27046c3b90 on Nov 6, 2021

kittywhiskers referenced this in commit ebb0c4f6bf on Nov 6, 2021

kittywhiskers referenced this in commit 179b6c5811 on Nov 6, 2021

kittywhiskers referenced this in commit 36f9303f8b on Nov 6, 2021

kittywhiskers referenced this in commit 6bd7f40814 on Nov 6, 2021

kittywhiskers referenced this in commit aa6ca7a730 on Nov 6, 2021

kittywhiskers referenced this in commit ea196a826d on Nov 6, 2021

kittywhiskers referenced this in commit deaf6b00ef on Nov 6, 2021

kittywhiskers referenced this in commit adb5e3d338 on Nov 6, 2021

kittywhiskers referenced this in commit ea06c363c1 on Nov 6, 2021

kittywhiskers referenced this in commit 4fc7c8533c on Nov 6, 2021

kittywhiskers referenced this in commit bf8b2039e7 on Nov 6, 2021

kittywhiskers referenced this in commit 5ab5c74af2 on Nov 6, 2021

kittywhiskers referenced this in commit b67fead2bb on Nov 11, 2021

kittywhiskers referenced this in commit 5cf9908bb5 on Nov 11, 2021

kittywhiskers referenced this in commit ea0227efc6 on Nov 11, 2021

kittywhiskers referenced this in commit a43c404d9e on Nov 11, 2021

kittywhiskers referenced this in commit aa48c2f279 on Nov 11, 2021

kittywhiskers referenced this in commit b819bbd74b on Nov 11, 2021

kittywhiskers referenced this in commit 715a98860b on Nov 11, 2021

kittywhiskers referenced this in commit dbcc19e931 on Nov 11, 2021

PastaPastaPasta referenced this in commit 305b96e008 on Nov 13, 2021

pravblockc referenced this in commit 214b467346 on Nov 18, 2021

ftrader referenced this in commit f6aeed9f2d on Nov 21, 2021

ftrader referenced this in commit 7ef10ed78d on Nov 21, 2021

ftrader referenced this in commit e984047ab3 on Nov 21, 2021

ftrader referenced this in commit 471e445c0e on Nov 21, 2021

ftrader referenced this in commit d15caff12d on Nov 21, 2021

ftrader referenced this in commit f21614c9f0 on Nov 21, 2021

ftrader referenced this in commit 137233442f on Nov 21, 2021

DrahtBot locked this on Dec 16, 2021

Refactor: Add Flags enum to ArgsManager class #16097

Conflicts

Commit checklist

Tests