refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage

ryanofsky · ryanofsky · commit 6e613a0a3058 · 2021-09-27T09:13:09.000-04:00
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545). An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed. Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in two commits, with this commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
diff --git a/src/util/system.cpp b/src/util/system.cpp
@@ -227,7 +227,7 @@ static std::optional<util::SettingsValue> InterpretValue(const std::string& key,
 {
     // Return negated settings as false values.
     if (negated) {
-        if (!(flags & ArgsManager::ALLOW_BOOL)) {
+        if (flags & ArgsManager::DISALLOW_NEGATION) {
             error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);
             return std::nullopt;
         }
@@ -650,6 +650,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
 
     LOCK(cs_args);
     std::map<std::string, Arg>& arg_map = m_available_args[cat];
+    if ((flags & (ALLOW_ANY | ALLOW_BOOL)) == 0) flags |= DISALLOW_NEGATION; // Temporary, removed in next scripted-diff
     auto ret = arg_map.emplace(arg_name, Arg{name.substr(eq_index, name.size() - eq_index), help, flags});
     assert(ret.second); // Make sure an insertion actually happened
 
diff --git a/src/util/system.h b/src/util/system.h
@@ -158,12 +158,18 @@ struct SectionInfo
 class ArgsManager
 {
 public:
+    /**
+     * Flags controlling how config and command line arguments are validated and
+     * interpreted.
+     */
     enum Flags : uint32_t {
-        // Boolean options can accept negation syntax -noOPTION or -noOPTION=1
-        ALLOW_BOOL = 0x01,
-        ALLOW_INT = 0x02,
-        ALLOW_STRING = 0x04,
-        ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
+        ALLOW_ANY = 0x01,         //!< disable validation
+        ALLOW_BOOL = 0x02,        //!< unimplemented, draft implementation in #16545
+        ALLOW_INT = 0x04,         //!< unimplemented, draft implementation in #16545
+        ALLOW_STRING = 0x08,      //!< unimplemented, draft implementation in #16545
+        ALLOW_LIST = 0x10,        //!< unimplemented, draft implementation in #16545
+        DISALLOW_NEGATION = 0x20, //!< disallow -nofoo syntax
+
         DEBUG_ONLY = 0x100,
         /* Some options would cause cross-contamination if values for
          * mainnet were used while running on regtest/testnet (or vice-versa).

Original file line number	Diff line number	Diff line change
`@@ -227,7 +227,7 @@ static std::optional<util::SettingsValue> InterpretValue(const std::string& key,`
`227`	`227`	`{`
`228`	`228`	`// Return negated settings as false values.`
`229`	`229`	`if (negated) {`
`230`		`- if (!(flags & ArgsManager::ALLOW_BOOL)) {`
	`230`	`+ if (flags & ArgsManager::DISALLOW_NEGATION) {`
`231`	`231`	`error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);`
`232`	`232`	`return std::nullopt;`
`233`	`233`	`}`
`@@ -650,6 +650,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig`
`650`	`650`
`651`	`651`	`LOCK(cs_args);`
`652`	`652`	`std::map<std::string, Arg>& arg_map = m_available_args[cat];`
	`653`	`+ if ((flags & (ALLOW_ANY \| ALLOW_BOOL)) == 0) flags \|= DISALLOW_NEGATION; // Temporary, removed in next scripted-diff`
`653`	`654`	`auto ret = arg_map.emplace(arg_name, Arg{name.substr(eq_index, name.size() - eq_index), help, flags});`
`654`	`655`	`assert(ret.second); // Make sure an insertion actually happened`
`655`	`656`