================
@@ -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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to