[Lldb-commits] [lldb] [lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)

[Greg Clayton via lldb-commits]

================
@@ -1666,33 +1646,52 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
     // the user's options.
     ProcessSP process_sp = target.GetProcessSP();
 
-    int stop_action = -1;   // -1 means leave the current setting alone
-    int pass_action = -1;   // -1 means leave the current setting alone
-    int notify_action = -1; // -1 means leave the current setting alone
+    std::optional<bool> stop_action = {};
+    std::optional<bool> pass_action = {};
+    std::optional<bool> notify_action = {};
 
-    if (!m_options.stop.empty() &&
-        !VerifyCommandOptionValue(m_options.stop, stop_action)) {
-      result.AppendError("Invalid argument for command option --stop; must be "
-                         "true or false.\n");
-      return;
+    if (!m_options.stop.empty()) {
+      bool success = false;
+      bool value = OptionArgParser::ToBoolean(m_options.stop, false, &success);
----------------
clayborg wrote:

Why don't we do this error checking in `CommandObjectProcessHandle::CommandOptions::SetOptionValue(...)`? We can return an error from there instead of making it successfully though option parsing only to look closer at the option values and return an error later in this function?

Also, I see many places use this version of `OptionArgParser::ToBoolean(...)`, and there is a ton of duplicated code like:
- call `OptionArgParser::ToBoolean(...)`
- check value of success
- make error string and everyone picks their own "hey this isn't a valid boolean value (like Jim already mentioned below).

Since there are already so many uses of the current and only version of `OptionArgParser::ToBoolean(...)`, maybe we make an overload of this:

```
llvm::Expected<bool> OptionArgParser::ToBoolean(llvm::StringRef ref);
```
Then in the `CommandObjectProcessHandle::CommandOptions::SetOptionValue(...) function we can do:
```
case 's':
  if (auto ExpectedBool = OptionArgParser::ToBoolean(option_arg)) {
    stop = *ExpectedBool;
  else
    error = Status(ExpectedBool.takeError());
```
And the new `OptionArgParser::ToBoolean()` overload would return an error as Jim wants below:
```
"Invalid <boolean> value "%s"
```
Where %s gets substitued with "option_arg.str().c_str()". And then the help would help people figure this out.

https://github.com/llvm/llvm-project/pull/79901