[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)

Alex Langford via lldb-commits lldb-commits at lists.llvm.org
Mon Jan 29 17:16:06 PST 2024


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

>From 3cd6afa16fb8b282712b1d3cf103abbd3329fc17 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Mon, 29 Jan 2024 13:15:24 -0800
Subject: [PATCH 1/3] [lldb][NFCI] Minor refactor to
 CommandObjectProcessHandle::VerifyCommandOptionValue

I was refactoring something else but ran into this function. It was
somewhat confusing to read through and understand, but it boils down to
two steps:
- First we try `OptionArgParser::ToBoolean`. If that works, then we're
  good to go.
- Second, we try `llvm::to_integer` to see if it's an integer. If it
  parses to 0 or 1, we're good.
- Failing either of the steps above means we cannot parse it into a
  bool.

Instead of having an integer out param and a bool return value, it seems
like the interface is better served with an optional<bool> -- Either it
parses into true or false, or you get back nothing (nullopt).
---
 lldb/source/Commands/CommandObjectProcess.cpp | 103 ++++++++----------
 1 file changed, 48 insertions(+), 55 deletions(-)

diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index c7b874d197937..f089a86275dc6 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,24 +1591,24 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
 
   Options *GetOptions() override { return &m_options; }
 
-  bool VerifyCommandOptionValue(const std::string &option, int &real_value) {
-    bool okay = true;
+  std::optional<bool> VerifyCommandOptionValue(const std::string &option) {
+    if (option.empty())
+      return std::nullopt;
+
     bool success = false;
     bool tmp_value = OptionArgParser::ToBoolean(option, false, &success);
+    if (success)
+      return tmp_value;
+
+    int parsed_value = -1;
+    if (llvm::to_integer(option, parsed_value)) {
+      if (parsed_value != 0 && parsed_value != 1)
+        return std::nullopt;
 
-    if (success && tmp_value)
-      real_value = 1;
-    else if (success && !tmp_value)
-      real_value = 0;
-    else {
-      // If the value isn't 'true' or 'false', it had better be 0 or 1.
-      if (!llvm::to_integer(option, real_value))
-        real_value = 3;
-      if (real_value != 0 && real_value != 1)
-        okay = false;
+      return parsed_value == 0 ? false : true;
     }
 
-    return okay;
+    return std::nullopt;
   }
 
   void PrintSignalHeader(Stream &str) {
@@ -1666,33 +1666,31 @@ 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 = VerifyCommandOptionValue(m_options.stop);
+    std::optional<bool> pass_action = VerifyCommandOptionValue(m_options.pass);
+    std::optional<bool> notify_action =
+        VerifyCommandOptionValue(m_options.notify);
 
-    if (!m_options.stop.empty() &&
-        !VerifyCommandOptionValue(m_options.stop, stop_action)) {
+    if (!m_options.stop.empty() && !stop_action.has_value()) {
       result.AppendError("Invalid argument for command option --stop; must be "
                          "true or false.\n");
       return;
     }
 
-    if (!m_options.notify.empty() &&
-        !VerifyCommandOptionValue(m_options.notify, notify_action)) {
-      result.AppendError("Invalid argument for command option --notify; must "
-                         "be true or false.\n");
+    if (!m_options.pass.empty() && !pass_action.has_value()) {
+      result.AppendError("Invalid argument for command option --pass; must be "
+                         "true or false.\n");
       return;
     }
 
-    if (!m_options.pass.empty() &&
-        !VerifyCommandOptionValue(m_options.pass, pass_action)) {
-      result.AppendError("Invalid argument for command option --pass; must be "
-                         "true or false.\n");
+    if (!m_options.notify.empty() && !notify_action.has_value()) {
+      result.AppendError("Invalid argument for command option --notify; must "
+                         "be true or false.\n");
       return;
     }
 
-    bool no_actions = (stop_action == -1 && pass_action == -1
-        && notify_action == -1);
+    bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
+                       !notify_action.has_value());
     if (m_options.only_target_values && !no_actions) {
       result.AppendError("-t is for reporting, not setting, target values.");
       return;
@@ -1732,14 +1730,14 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
           if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
             // Casting the actions as bools here should be okay, because
             // VerifyCommandOptionValue guarantees the value is either 0 or 1.
-            if (stop_action != -1)
-              signals_sp->SetShouldStop(signo, stop_action);
-            if (pass_action != -1) {
-              bool suppress = !pass_action;
+            if (stop_action.has_value())
+              signals_sp->SetShouldStop(signo, *stop_action);
+            if (pass_action.has_value()) {
+              bool suppress = !*pass_action;
               signals_sp->SetShouldSuppress(signo, suppress);
             }
-            if (notify_action != -1)
-              signals_sp->SetShouldNotify(signo, notify_action);
+            if (notify_action.has_value())
+              signals_sp->SetShouldNotify(signo, *notify_action);
             ++num_signals_set;
           } else {
             result.AppendErrorWithFormat("Invalid signal name '%s'\n",
@@ -1759,40 +1757,35 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
           }
          num_signals_set = num_args;
         }
-        auto set_lazy_bool = [] (int action) -> LazyBool {
-          LazyBool lazy;
-          if (action == -1)
-            lazy = eLazyBoolCalculate;
-          else if (action)
-            lazy = eLazyBoolYes;
-          else
-            lazy = eLazyBoolNo;
-          return lazy;
+        auto set_lazy_bool = [](std::optional<bool> action) -> LazyBool {
+          if (!action.has_value())
+            return eLazyBoolCalculate;
+          return (*action) ? eLazyBoolYes : eLazyBoolNo;
         };
 
         // If there were no actions, we're just listing, don't add the dummy:
         if (!no_actions)
-          target.AddDummySignal(arg.ref(),
-                                set_lazy_bool(pass_action),
-                                set_lazy_bool(notify_action),
-                                set_lazy_bool(stop_action));
+         target.AddDummySignal(arg.ref(), set_lazy_bool(pass_action),
+                               set_lazy_bool(notify_action),
+                               set_lazy_bool(stop_action));
       }
     } else {
       // No signal specified, if any command options were specified, update ALL
       // signals.  But we can't do this without a process since we don't know
       // all the possible signals that might be valid for this target.
-      if (((notify_action != -1) || (stop_action != -1) || (pass_action != -1))
-          && process_sp) {
+      if ((notify_action.has_value() || stop_action.has_value() ||
+           pass_action.has_value()) &&
+          process_sp) {
         if (m_interpreter.Confirm(
                 "Do you really want to update all the signals?", false)) {
           int32_t signo = signals_sp->GetFirstSignalNumber();
           while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
-            if (notify_action != -1)
-              signals_sp->SetShouldNotify(signo, notify_action);
-            if (stop_action != -1)
-              signals_sp->SetShouldStop(signo, stop_action);
-            if (pass_action != -1) {
-              bool suppress = !pass_action;
+            if (notify_action.has_value())
+              signals_sp->SetShouldNotify(signo, *notify_action);
+            if (stop_action.has_value())
+              signals_sp->SetShouldStop(signo, *stop_action);
+            if (pass_action.has_value()) {
+              bool suppress = !*pass_action;
               signals_sp->SetShouldSuppress(signo, suppress);
             }
             signo = signals_sp->GetNextSignalNumber(signo);

>From a93faa93b4f743f9b153909b193407a3078ce204 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Mon, 29 Jan 2024 13:29:23 -0800
Subject: [PATCH 2/3] Formatting

---
 lldb/source/Commands/CommandObjectProcess.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index f089a86275dc6..890c7314ba7bf 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1765,9 +1765,9 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
 
         // If there were no actions, we're just listing, don't add the dummy:
         if (!no_actions)
-         target.AddDummySignal(arg.ref(), set_lazy_bool(pass_action),
-                               set_lazy_bool(notify_action),
-                               set_lazy_bool(stop_action));
+          target.AddDummySignal(arg.ref(), set_lazy_bool(pass_action),
+                                set_lazy_bool(notify_action),
+                                set_lazy_bool(stop_action));
       }
     } else {
       // No signal specified, if any command options were specified, update ALL

>From 8e784550e50d9f93f3f02b75ecfdf60a977a2f80 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Mon, 29 Jan 2024 17:15:49 -0800
Subject: [PATCH 3/3] [lldb] Remove VerifyCommandOptionValue entirely

---
 lldb/source/Commands/CommandObjectProcess.cpp | 73 +++++++++----------
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 890c7314ba7bf..722b0e0c376be 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1591,26 +1591,6 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
 
   Options *GetOptions() override { return &m_options; }
 
-  std::optional<bool> VerifyCommandOptionValue(const std::string &option) {
-    if (option.empty())
-      return std::nullopt;
-
-    bool success = false;
-    bool tmp_value = OptionArgParser::ToBoolean(option, false, &success);
-    if (success)
-      return tmp_value;
-
-    int parsed_value = -1;
-    if (llvm::to_integer(option, parsed_value)) {
-      if (parsed_value != 0 && parsed_value != 1)
-        return std::nullopt;
-
-      return parsed_value == 0 ? false : true;
-    }
-
-    return std::nullopt;
-  }
-
   void PrintSignalHeader(Stream &str) {
     str.Printf("NAME         PASS   STOP   NOTIFY\n");
     str.Printf("===========  =====  =====  ======\n");
@@ -1666,27 +1646,48 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
     // the user's options.
     ProcessSP process_sp = target.GetProcessSP();
 
-    std::optional<bool> stop_action = VerifyCommandOptionValue(m_options.stop);
-    std::optional<bool> pass_action = VerifyCommandOptionValue(m_options.pass);
-    std::optional<bool> notify_action =
-        VerifyCommandOptionValue(m_options.notify);
+    std::optional<bool> stop_action = {};
+    std::optional<bool> pass_action = {};
+    std::optional<bool> notify_action = {};
 
-    if (!m_options.stop.empty() && !stop_action.has_value()) {
-      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);
+      if (!success) {
+        result.AppendError(
+            "Invalid argument for command option --stop; must be "
+            "true or false.\n");
+        return;
+      }
+
+      stop_action = value;
     }
 
-    if (!m_options.pass.empty() && !pass_action.has_value()) {
-      result.AppendError("Invalid argument for command option --pass; must be "
-                         "true or false.\n");
-      return;
+    if (!m_options.pass.empty()) {
+      bool success = false;
+      bool value = OptionArgParser::ToBoolean(m_options.pass, false, &success);
+      if (!success) {
+        result.AppendError(
+            "Invalid argument for command option --pass; must be "
+            "true or false.\n");
+        return;
+      }
+      pass_action = value;
+    }
+
+    if (!m_options.notify.empty()) {
+      bool success = false;
+      bool value =
+          OptionArgParser::ToBoolean(m_options.notify, false, &success);
+      if (!success) {
+        result.AppendError("Invalid argument for command option --notify; must "
+                           "be true or false.\n");
+        return;
+      }
+      notify_action = value;
     }
 
     if (!m_options.notify.empty() && !notify_action.has_value()) {
-      result.AppendError("Invalid argument for command option --notify; must "
-                         "be true or false.\n");
-      return;
     }
 
     bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
@@ -1728,8 +1729,6 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
         if (signals_sp) {
           int32_t signo = signals_sp->GetSignalNumberFromName(arg.c_str());
           if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
-            // Casting the actions as bools here should be okay, because
-            // VerifyCommandOptionValue guarantees the value is either 0 or 1.
             if (stop_action.has_value())
               signals_sp->SetShouldStop(signo, *stop_action);
             if (pass_action.has_value()) {



More information about the lldb-commits mailing list