Get rid of some uses of StringConvert and reduce some indentation

[zturner]

Summary

The goal here is to eventually delete the StringConvert class, as the llvm APIs for doing string to number conversion have a cleaner syntax.

In order to reduce code churn, since I was in this code anyway, I changed the deeply nested indentation of the code blocks I was touching to use early outs. This led me to get annoyed by the fact that every time we want to return something, it has to be 3 or 4 lines of code to open a scope, set the correct status on the error, set the message, eventually return, and then add another line of code to close the scope. We should be able to do all this in one line to further reduce boilerplate goo. So I added some code to Status to do this. Whereas before we would have to write this:

if (!succeed) {
  result.AppendError("some error");
  result.SetStatus(eStatusFailed);
  return result.Succeeded();
}

which is 5 lines of code, now we only have to write:

if (!success)
  return result.AppendError(eStatusFailed, "some error");

which is only 2 lines.

There are more occurrences of StringConvert to change, but I wanted to get the feedback out of the way first before I go and do the rest.

[krytarowski]

It's not that perfectly clear to me that SetStatus() returns Succeeded(), however I don't have a proposal for a better name. SetStatusReturnSucceeded() or similar are long.

[labath]

I think this is a step in the right direction. Besides reducing boilerplate, this will also help us ensure correctness, as we get a constant trickle of bug reports for commands which forgot to set the result status.

The name is not ideal, but it's probably the best we can get. (The ideal solution for me would be to get rid of the duality in the DoExecute function -- e.g. remove the bool return and let the execution state be fully signalled by the result object -- but that's way off from what you were originally trying to do).

That said, I'm not sure I should be the person approving this. :)

lldb/source/Commands/CommandObjectPlatform.cpp

611

I believe this should be "{0}", error to avoid hitting problems in case the error contains '{'. (Plus it's 5 chars shorter :) )

lldb/source/Commands/CommandObjectProcess.cpp

1181

AsCString unnecessary

[jingham]

This looks fine. Like Kamil, I think it would help to document your new interfaces.

Going away from StringConvert, you are switching from an API that gives a value on fail to one that doesn't change the input value on error. You mostly handle the failures right away, so in that case it's fine to just declare them. But anywhere it's not immediately clear you are directly returning, you should probably initialize the variables to the fail value.

lldb/include/lldb/Interpreter/CommandReturnObject.h

132–154

Can you add a comment saying what these functions return? It's clear now that the code is in the header file, but that doesn't describe a contract just a current state, and anyway, will keep it clear if the code gets moved to the implementation file.

lldb/source/Commands/CommandObjectProcess.cpp

1178–1181

"ST" is not how we name variables in lldb. Besides the capitalization, which is wrong, we used to call all Error variables "error" which gave its use a bit of consistency. You didn't change the variable names when you renamed them to "error" which is probably why calling this one "error" gave you pause. But we should maintain consistency for now, and that will make it easier to go rename them to whatever is appropriate when we get around to that.

lldb/source/Commands/CommandObjectTarget.cpp

2687–2688

I don't think it does any harm here. But your switching from an API that sets a fail value to one that doesn't, so you're adding the possibility of uninitialized variable access. Probably good as you are making this transition to initialize to the fail value.

lldb/include/lldb/Interpreter/CommandReturnObject.h

void AppendWarningWithFormatv(const char *format, Args &&... args) {
     AppendWarning(llvm::formatv(format, std::forward<Args>(args)...).str());
   }
 
   template <typename... Args>
   void AppendErrorWithFormatv(const char *format, Args &&... args) {
     AppendError(llvm::formatv(format, std::forward<Args>(args)...).str());
   }
 
+  template <typename... Args>
+  bool AppendError(lldb::ReturnStatus Status, const char *Format,
+                   Args &&... args) {
+    SetStatus(Status);
+    AppendErrorWithFormatv(Format, std::forward<Args>(args)...);
+    return Succeeded();
+  }
+
+  template <typename... Args>
+  bool AppendMessage(lldb::ReturnStatus Status, const char *Format,
+                     Args &&... args) {
+    SetStatus(Status);
+    AppendMessageWithFormatv(Format, std::forward<Args>(args)...);
+    return Succeeded();
+  }
+
   void SetError(const Status &error, const char *fallback_error_cstr = nullptr);
 
   void SetError(llvm::StringRef error_cstr);
 
   lldb::ReturnStatus GetStatus();
 
-  void SetStatus(lldb::ReturnStatus status);
+  bool SetStatus(lldb::ReturnStatus status);
 
   bool Succeeded();
 
   bool HasResult();
 
   bool GetDidChangeProcessState();
 
   void SetDidChangeProcessState(bool b);

lldb/include/lldb/Utility/Status.h

public:
   /// @param[in] err
   ///     A mach error code.
   ///
   /// @return
   ///     A const reference to this object.
   //------------------------------------------------------------------
   const Status &operator=(uint32_t err);
 
+  operator bool() const { return Fail(); }
+
   ~Status();
 
   //------------------------------------------------------------------
   /// Get the error string associated with the current error.
   //
   /// Gets the error value as a NULL terminated C string. The error
   /// string will be fetched and cached on demand. The error string
   /// will be retrieved from a callback that is appropriate for the

lldb/source/Commands/CommandObjectPlatform.cpp

 #include <mutex>
 // Other libraries and framework includes
 // Project includes
 #include "CommandObjectPlatform.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/OptionParser.h"
-#include "lldb/Host/StringConvert.h"
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandOptionValidators.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionGroupFile.h"
 #include "lldb/Interpreter/OptionGroupPlatform.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/DataExtractor.h"
 
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Threading.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static mode_t ParsePermissionString(const char *) = delete;
 
 static mode_t ParsePermissionString(llvm::StringRef permissions) {
CommandObjectPlatformFClose(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "platform file close",
                             "Close a file on the remote end.", nullptr, 0) {}
 
   ~CommandObjectPlatformFClose() override = default;
 
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     PlatformSP platform_sp(
         m_interpreter.GetDebugger().GetPlatformList().GetSelectedPlatform());
-    if (platform_sp) {
-      std::string cmd_line;
-      args.GetCommandString(cmd_line);
-      const lldb::user_id_t fd =
-          StringConvert::ToUInt64(cmd_line.c_str(), UINT64_MAX);
-      Status error;
-      bool success = platform_sp->CloseFile(fd, error);
-      if (success) {
-        result.AppendMessageWithFormat("file %" PRIu64 " closed.\n", fd);
-        result.SetStatus(eReturnStatusSuccessFinishResult);
-      } else {
-        result.AppendError(error.AsCString());
-        result.SetStatus(eReturnStatusFailed);
-      }
-    } else {
-      result.AppendError("no platform currently selected\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+    if (!platform_sp)
+      return result.AppendError(eReturnStatusFailed,
+                                "no platform currently selected.\n");
+
+    lldb::user_id_t fd;
+    if (!llvm::strton(cmd_line, fd))
+      return result.AppendError(eReturnStatusFailed, "Invalid fd {0}",
+                                cmd_line);
+
+    if (!platform_sp->CloseFile(fd, error))
+      return result.AppendError(eReturnStatusFailed, error.AsCString());
+
+    return result.AppendMessage(eReturnStatusSuccessFinishResult,
+                                "file {0} closed.\n", fd);
   }
 };
 
 //----------------------------------------------------------------------
 // "platform fread"
 //----------------------------------------------------------------------
 
 static OptionDefinition g_platform_fread_options[] = {
CommandObjectPlatformFRead(CommandInterpreter &interpreter)
                             0),
         m_options() {}
 
   ~CommandObjectPlatformFRead() override = default;
 
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     PlatformSP platform_sp(
         m_interpreter.GetDebugger().GetPlatformList().GetSelectedPlatform());
-    if (platform_sp) {
-      std::string cmd_line;
-      args.GetCommandString(cmd_line);
-      const lldb::user_id_t fd =
-          StringConvert::ToUInt64(cmd_line.c_str(), UINT64_MAX);
-      std::string buffer(m_options.m_count, 0);
-      Status error;
-      uint32_t retcode = platform_sp->ReadFile(
-          fd, m_options.m_offset, &buffer[0], m_options.m_count, error);
-      result.AppendMessageWithFormat("Return = %d\n", retcode);
-      result.AppendMessageWithFormat("Data = \"%s\"\n", buffer.c_str());
-      result.SetStatus(eReturnStatusSuccessFinishResult);
-    } else {
-      result.AppendError("no platform currently selected\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+    if (!platform_sp)
+      return result.AppendError(eReturnStatusFailed,
+                                "no platform currently selected\n");
+
+    lldb::user_id_t fd;
+    if (!llvm::strton(cmd_line, fd))
+      return result.AppendError(eReturnStatusFailed,
+                                "Invalid file descriptor {0}\n", fd);
+    uint32_t retcode = platform_sp->ReadFile(fd, m_options.m_offset, &buffer[0],
+                                             m_options.m_count, error);
+    return result.AppendMessage(eReturnStatusSuccessFinishResult,
+                                "Return = {0}\nData = \"{1}\"\n", retcode,
+                                buffer);
   }
 
   Options *GetOptions() override { return &m_options; }
 
 protected:
   class CommandOptions : public Options {
   public:
     CommandOptions() : Options() {}
CommandObjectPlatformFWrite(CommandInterpreter &interpreter)
                             0),
         m_options() {}
 
   ~CommandObjectPlatformFWrite() override = default;
 
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     PlatformSP platform_sp(
         m_interpreter.GetDebugger().GetPlatformList().GetSelectedPlatform());
-    if (platform_sp) {
-      std::string cmd_line;
-      args.GetCommandString(cmd_line);
-      Status error;
-      const lldb::user_id_t fd =
-          StringConvert::ToUInt64(cmd_line.c_str(), UINT64_MAX);
-      uint32_t retcode =
-          platform_sp->WriteFile(fd, m_options.m_offset, &m_options.m_data[0],
-                                 m_options.m_data.size(), error);
-      result.AppendMessageWithFormat("Return = %d\n", retcode);
-      result.SetStatus(eReturnStatusSuccessFinishResult);
-    } else {
-      result.AppendError("no platform currently selected\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+    if (!platform_sp)
+      return result.AppendError(eReturnStatusFailed,
+                                "no platform currently selected\n");
+
+    lldb::user_id_t fd;
+    if (!llvm::strton(cmd_line, fd))
+      return result.AppendError(eReturnStatusFailed,
+                                "Invalid file descriptor {0}\n", cmd_line);
+
+    return result.AppendMessage(eReturnStatusSuccessFinishResult,
+                                "Return = {0}\n", retcode);
   }
 
   Options *GetOptions() override { return &m_options; }
 
 protected:
   class CommandOptions : public Options {
   public:
     CommandOptions() : Options() {}

lldb/source/Commands/CommandObjectProcess.cpp

 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointSite.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/State.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/OptionParser.h"
-#include "lldb/Host/StringConvert.h"
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/UnixSignals.h"
 
+#include "llvm/ADT/StringExtras.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
 class CommandObjectProcessLaunchOrAttach : public CommandObjectParsed {
 public:
   CommandObjectProcessLaunchOrAttach(CommandInterpreter &interpreter,
                                      const char *name, const char *help,
                                      const char *syntax, uint32_t flags,
public:
   }
 
   ~CommandObjectProcessSignal() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Process *process = m_exe_ctx.GetProcessPtr();
 
-    if (command.GetArgumentCount() == 1) {
-      int signo = LLDB_INVALID_SIGNAL_NUMBER;
+    if (command.GetArgumentCount() != 1) {
+      return result.AppendError(
+          eReturnStatusFailed,
+          "'{0}' takes exactly one signal number argument:\nUsage: {1}\n",
+          m_cmd_name, m_cmd_syntax);
+    }
+
 
-      const char *signal_name = command.GetArgumentAtIndex(0);
-      if (::isxdigit(signal_name[0]))
-        signo =
-            StringConvert::ToSInt32(signal_name, LLDB_INVALID_SIGNAL_NUMBER, 0);
-      else
-        signo = process->GetUnixSignals()->GetSignalNumberFromName(signal_name);
+      llvm::strton(signal_name, signo, 16);
 
-      if (signo == LLDB_INVALID_SIGNAL_NUMBER) {
-        result.AppendErrorWithFormat("Invalid signal argument '%s'.\n",
-                                     command.GetArgumentAtIndex(0));
-        result.SetStatus(eReturnStatusFailed);
-      } else {
-        Status error(process->Signal(signo));
-        if (error.Success()) {
-          result.SetStatus(eReturnStatusSuccessFinishResult);
-        } else {
-          result.AppendErrorWithFormat("Failed to send signal %i: %s\n", signo,
-                                       error.AsCString());
-          result.SetStatus(eReturnStatusFailed);
-        }
-      }
-    } else {
-      result.AppendErrorWithFormat(
-          "'%s' takes exactly one signal number argument:\nUsage: %s\n",
-          m_cmd_name.c_str(), m_cmd_syntax.c_str());
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+    if (signo == LLDB_INVALID_SIGNAL_NUMBER)
+      return result.AppendError(eReturnStatusFailed,
+                                "Invalid signal argument '{0}'.\n",
+                                command[0].ref);
+
+    if (auto ST = process->Signal(signo))
+      return result.AppendError(eReturnStatusFailed,
+                                "Failed to send signal {0}: {1}\n", signo,
+                                ST.AsCString());
+
+    return result.SetStatus(eReturnStatusSuccessFinishResult);
   }
 };
 
 //-------------------------------------------------------------------------
 // CommandObjectProcessInterrupt
 //-------------------------------------------------------------------------
 #pragma mark CommandObjectProcessInterrupt
 
CommandObjectProcessHandle(CommandInterpreter &interpreter)
     m_arguments.push_back(arg);
   }
 
   ~CommandObjectProcessHandle() override = default;
 
   Options *GetOptions() override { return &m_options; }
 
   bool VerifyCommandOptionValue(const std::string &option, int &real_value) {
-    bool okay = true;
     bool success = false;
     bool tmp_value = Args::StringToBoolean(option, false, &success);
 
-    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.
-      real_value = StringConvert::ToUInt32(option.c_str(), 3);
-      if (real_value != 0 && real_value != 1)
-        okay = false;
+    if (success) {
+      real_value = static_cast<int>(tmp_value);
+      return true;
     }
 
-    return okay;
+    // If the value isn't 'true' or 'false', it had better be 0 or 1.
+    return llvm::strton(option, real_value, 2);
   }
 
   void PrintSignalHeader(Stream &str) {
     str.Printf("NAME         PASS   STOP   NOTIFY\n");
     str.Printf("===========  =====  =====  ======\n");
   }
 
   void PrintSignal(Stream &str, int32_t signo, const char *sig_name,

lldb/source/Commands/CommandObjectTarget.cpp

 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/State.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Core/ValueObjectVariable.h"
 #include "lldb/DataFormatters/ValueObjectPrinter.h"
 #include "lldb/Host/OptionParser.h"
-#include "lldb/Host/StringConvert.h"
 #include "lldb/Host/Symbols.h"
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionGroupArchitecture.h"
 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Interpreter/OptionGroupFile.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"

 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadSpec.h"
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/FileSystem.h"
 
 // C Includes
 // C++ Includes
 #include <cerrno>
 
 using namespace lldb;
 using namespace lldb_private;
CommandObjectTargetSelect(CommandInterpreter &interpreter)
             interpreter, "target select",
             "Select a target as the current target by target index.", nullptr) {
   }
 
   ~CommandObjectTargetSelect() override = default;
 
 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
-    if (args.GetArgumentCount() == 1) {
-      bool success = false;
-      const char *target_idx_arg = args.GetArgumentAtIndex(0);
-      uint32_t target_idx =
-          StringConvert::ToUInt32(target_idx_arg, UINT32_MAX, 0, &success);
-      if (success) {
-        TargetList &target_list = m_interpreter.GetDebugger().GetTargetList();
-        const uint32_t num_targets = target_list.GetNumTargets();
-        if (target_idx < num_targets) {
-          TargetSP target_sp(target_list.GetTargetAtIndex(target_idx));
-          if (target_sp) {
-            Stream &strm = result.GetOutputStream();
-            target_list.SetSelectedTarget(target_sp.get());
-            bool show_stopped_process_status = false;
-            DumpTargetList(target_list, show_stopped_process_status, strm);
-            result.SetStatus(eReturnStatusSuccessFinishResult);
-          } else {
-            result.AppendErrorWithFormat("target #%u is NULL in target list\n",
-                                         target_idx);
-            result.SetStatus(eReturnStatusFailed);
-          }
-        } else {
-          if (num_targets > 0) {
-            result.AppendErrorWithFormat(
-                "index %u is out of range, valid target indexes are 0 - %u\n",
-                target_idx, num_targets - 1);
-          } else {
-            result.AppendErrorWithFormat(
-                "index %u is out of range since there are no active targets\n",
-                target_idx);
-          }
-          result.SetStatus(eReturnStatusFailed);
-        }
-      } else {
-        result.AppendErrorWithFormat("invalid index string value '%s'\n",
-                                     target_idx_arg);
-        result.SetStatus(eReturnStatusFailed);
-      }
-    } else {
-      result.AppendError(
-          "'target select' takes a single argument: a target index\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+    if (args.GetArgumentCount() != 1)
+      return result.AppendError(
+          eReturnStatusFailed,
+          "'target select' takes a single argument: a target index\n");
+
+    uint32_t target_idx;
+    if (!llvm::strton(args[0].ref, target_idx))
+      return result.AppendError(eReturnStatusFailed,
+                                "invalid index string value '{0}'\n",
+                                args[0].ref);
+
+    if (target_idx >= num_targets) {
+      constexpr auto S = eReturnStatusFailed;
+      if (num_targets > 0)
+        return result.AppendError(
+            S, "index {0} is out of range, valid target indices are 0 - {1}\n",
+            target_idx, num_targets - 1);
+      return result.AppendError(
+          S, "index {0} is out of range since there are no active targets\n",
+          target_idx);
+    }
+
+    if (!target_sp)
+      return result.AppendError(eReturnStatusFailed,
+                                "target #{0} is NULL in target list\n",
+                                target_idx);
+
+    return result.SetStatus(eReturnStatusSuccessFinishResult);
   }
 };
 
 #pragma mark CommandObjectTargetSelect
 
 //----------------------------------------------------------------------
 // "target delete"
 //----------------------------------------------------------------------
CommandObjectTargetModulesSearchPathsInsert(CommandInterpreter &interpreter)
     m_arguments.push_back(arg2);
   }
 
   ~CommandObjectTargetModulesSearchPathsInsert() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target *target = m_interpreter.GetDebugger().GetSelectedTarget().get();
-    if (target) {
-      size_t argc = command.GetArgumentCount();
-      // check for at least 3 arguments and an odd number of parameters
-      if (argc >= 3 && argc & 1) {
-        bool success = false;
+    if (!target)
+      return result.AppendError(eReturnStatusFailed, "invalid target\n");
+
+    if (argc < 3 || argc % 2 == 0)
+      return result.AppendError(eReturnStatusFailed,
+                                "insert requires at least three arguments\n");
 
-        uint32_t insert_idx = StringConvert::ToUInt32(
-            command.GetArgumentAtIndex(0), UINT32_MAX, 0, &success);
+    bool success = false;
 
-        if (!success) {
-          result.AppendErrorWithFormat(
-              "<index> parameter is not an integer: '%s'.\n",
-              command.GetArgumentAtIndex(0));
-          result.SetStatus(eReturnStatusFailed);
-          return result.Succeeded();
-        }
+    uint32_t insert_idx;
+    if (!llvm::strton(command[0].ref, insert_idx))
+      return result.AppendError(eReturnStatusFailed,
+                                "<index> parameter is not an integer: '{0}'.\n",
+                                command[0].ref);
 
-        // shift off the index
-        command.Shift();
-        argc = command.GetArgumentCount();
 
-        for (uint32_t i = 0; i < argc; i += 2, ++insert_idx) {
-          const char *from = command.GetArgumentAtIndex(i);
-          const char *to = command.GetArgumentAtIndex(i + 1);
 
-          if (from[0] && to[0]) {
-            bool last_pair = ((argc - i) == 2);
-            target->GetImageSearchPathList().Insert(
-                ConstString(from), ConstString(to), insert_idx, last_pair);
-            result.SetStatus(eReturnStatusSuccessFinishNoResult);
-          } else {
-            if (from[0])
-              result.AppendError("<path-prefix> can't be empty\n");
-            else
-              result.AppendError("<new-path-prefix> can't be empty\n");
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-        }
-      } else {
-        result.AppendError("insert requires at least three arguments\n");
-        result.SetStatus(eReturnStatusFailed);
-        return result.Succeeded();
-      }
-
-    } else {
-      result.AppendError("invalid target\n");
-      result.SetStatus(eReturnStatusFailed);
+      if (!to[0])
+        return result.AppendError(eReturnStatusFailed,
+                                  "<new-path-prefix> can't be empty\n");
+      if (!from[0])
+        return result.AppendError(eReturnStatusFailed,
+                                  "<path-prefix> can't be empty\n");
+
     }
     return result.Succeeded();
   }
 };
 
 #pragma mark CommandObjectTargetModulesSearchPathsList
 
 class CommandObjectTargetModulesSearchPathsList : public CommandObjectParsed {
if (target == nullptr) {
                                        "section load addresses.\n");
                     result.SetStatus(eReturnStatusFailed);
                     return false;
                   }
 
                   for (size_t i = 0; i < argc; i += 2) {
                     const char *sect_name = args.GetArgumentAtIndex(i);
                     const char *load_addr_cstr = args.GetArgumentAtIndex(i + 1);
-                    if (sect_name && load_addr_cstr) {
-                      ConstString const_sect_name(sect_name);
-                      bool success = false;
-                      addr_t load_addr = StringConvert::ToUInt64(
-                          load_addr_cstr, LLDB_INVALID_ADDRESS, 0, &success);
-                      if (success) {
-                        SectionSP section_sp(
-                            section_list->FindSectionByName(const_sect_name));
-                        if (section_sp) {
-                          if (section_sp->IsThreadSpecific()) {
-                            result.AppendErrorWithFormat(
-                                "thread specific sections are not yet "
-                                "supported (section '%s')\n",
-                                sect_name);
-                            result.SetStatus(eReturnStatusFailed);
-                            break;
-                          } else {
-                            if (target->GetSectionLoadList()
-                                    .SetSectionLoadAddress(section_sp,
-                                                           load_addr))
-                              changed = true;
-                            result.AppendMessageWithFormat(
-                                "section '%s' loaded at 0x%" PRIx64 "\n",
-                                sect_name, load_addr);
-                          }
-                        } else {
-                          result.AppendErrorWithFormat("no section found that "
-                                                       "matches the section "
-                                                       "name '%s'\n",
-                                                       sect_name);
-                          result.SetStatus(eReturnStatusFailed);
-                          break;
-                        }
-                      } else {
-                        result.AppendErrorWithFormat(
-                            "invalid load address string '%s'\n",
-                            load_addr_cstr);
-                        result.SetStatus(eReturnStatusFailed);
-                        break;
-                      }
-                    } else {
-                      if (sect_name)
-                        result.AppendError("section names must be followed by "
-                                           "a load address.\n");
-                      else
-                        result.AppendError("one or more section name + load "
-                                           "address pair must be specified.\n");
-                      result.SetStatus(eReturnStatusFailed);
-                      break;
-                    }
+                    if (!sect_name) {
+                      result.AppendError(eReturnStatusFailed,
+                                         "one or more section name + load "
+                                         "address pair must be specified.\n");
+                      break;
+                    }
+                    if (!load_addr_cstr) {
+                      result.AppendError("section names must be followed by "
+                                         "a load address.\n");
+                      break;
+                    }
+
+                    addr_t load_addr;
+                    if (!llvm::strton(load_addr_cstr, load_addr)) {
+                      result.AppendError(eReturnStatusFailed,
+                                         "invalid load address string '{0}'\n",
+                                         load_addr_cstr);
+                      break;
+                    }
+
+                    if (!section_sp) {
+                      result.AppendError(eReturnStatusFailed,
+                                         "no section found that "
+                                         "matches the section name '{0}'\n",
+                                         sect_name);
+                      break;
+                    }
+
+                    }
+
+                    if (target->GetSectionLoadList().SetSectionLoadAddress(
+                            section_sp, load_addr))
+                        "section '%s' loaded at 0x%" PRIx64 "\n", sect_name,
+                        load_addr);
                   }
                 }
 
                 if (changed) {
                   target->ModulesDidLoad(matching_modules);
                   Process *process = m_exe_ctx.GetProcessPtr();
                   if (process)
                     process->Flush();
CommandObjectTargetStopHookDelete(CommandInterpreter &interpreter)
                             "Delete a stop-hook.",
                             "target stop-hook delete [<idx>]") {}
 
   ~CommandObjectTargetStopHookDelete() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target *target = GetSelectedOrDummyTarget();
-    if (target) {
-      // FIXME: see if we can use the breakpoint id style parser?
-      size_t num_args = command.GetArgumentCount();
-      if (num_args == 0) {
-        if (!m_interpreter.Confirm("Delete all stop hooks?", true)) {
-          result.SetStatus(eReturnStatusFailed);
-          return false;
-        } else {
-          target->RemoveAllStopHooks();
-        }
-      } else {
-        bool success;
-        for (size_t i = 0; i < num_args; i++) {
-          lldb::user_id_t user_id = StringConvert::ToUInt32(
-              command.GetArgumentAtIndex(i), 0, 0, &success);
-          if (!success) {
-            result.AppendErrorWithFormat("invalid stop hook id: \"%s\".\n",
-                                         command.GetArgumentAtIndex(i));
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-          success = target->RemoveStopHookByID(user_id);
-          if (!success) {
-            result.AppendErrorWithFormat("unknown stop hook id: \"%s\".\n",
-                                         command.GetArgumentAtIndex(i));
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-        }
-      }
-      result.SetStatus(eReturnStatusSuccessFinishNoResult);
-    } else {
-      result.AppendError("invalid target\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-
-    return result.Succeeded();
+    if (!target)
+      return result.AppendError(eReturnStatusFailed, "invalid target\n");
+
+      if (!m_interpreter.Confirm("Delete all stop hooks?", true))
+        return result.SetStatus(eReturnStatusFailed);
+      for (const auto &arg : command) {
+        lldb::user_id_t user_id;
+        if (!llvm::strton(arg.ref, user_id))
+          return result.AppendError(
+              eReturnStatusFailed, "invalid stop hook id: \"{0}\".\n", arg.ref);
+
+        if (!success)
+          return result.AppendError(
+              eReturnStatusFailed, "unknown stop hook id: \"{0}\".\n", arg.ref);
+    return result.SetStatus(eReturnStatusSuccessFinishNoResult);
   }
 };
 
 #pragma mark CommandObjectTargetStopHookEnableDisable
 
 //-------------------------------------------------------------------------
 // CommandObjectTargetStopHookEnableDisable
 //-------------------------------------------------------------------------
 
 class CommandObjectTargetStopHookEnableDisable : public CommandObjectParsed {
 public:
   CommandObjectTargetStopHookEnableDisable(CommandInterpreter &interpreter,
                                            bool enable, const char *name,
                                            const char *help, const char *syntax)
       : CommandObjectParsed(interpreter, name, help, syntax), m_enable(enable) {
   }
 
   ~CommandObjectTargetStopHookEnableDisable() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target *target = GetSelectedOrDummyTarget();
-    if (target) {
-      // FIXME: see if we can use the breakpoint id style parser?
-      size_t num_args = command.GetArgumentCount();
-      bool success;
+    if (!target)
+      return result.AppendError(eReturnStatusFailed, "invalid target\n");
+
 
-      if (num_args == 0) {
-        target->SetAllStopHooksActiveState(m_enable);
-      } else {
-        for (size_t i = 0; i < num_args; i++) {
-          lldb::user_id_t user_id = StringConvert::ToUInt32(
-              command.GetArgumentAtIndex(i), 0, 0, &success);
-          if (!success) {
-            result.AppendErrorWithFormat("invalid stop hook id: \"%s\".\n",
-                                         command.GetArgumentAtIndex(i));
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-          success = target->SetStopHookActiveStateByID(user_id, m_enable);
-          if (!success) {
-            result.AppendErrorWithFormat("unknown stop hook id: \"%s\".\n",
-                                         command.GetArgumentAtIndex(i));
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-        }
-      }
-      result.SetStatus(eReturnStatusSuccessFinishNoResult);
-    } else {
-      result.AppendError("invalid target\n");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+      return result.SetStatus(eReturnStatusSuccessFinishNoResult);
+
+    for (const auto &arg : command) {
+      lldb::user_id_t user_id;
+      if (!llvm::strton(arg.ref, user_id))
+        result.AppendError(eReturnStatusFailed,
+                           "invalid stop hook id: \"{0}\".\n", arg.ref);
+
+      if (!target->SetStopHookActiveStateByID(user_id, m_enable))
+        return result.AppendError(eReturnStatusFailed,
+                                  "unknown stop hook id: \"{0}\".\n", arg.ref);
+    return result.SetStatus(eReturnStatusSuccessFinishNoResult);
   }
 
 private:
   bool m_enable;
 };
 
 #pragma mark CommandObjectTargetStopHookList
 

lldb/source/Interpreter/CommandReturnObject.cpp

 // don't append "\n" to the end of it.
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   GetErrorStream() << in_string;
 }
 
-void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }
+bool CommandReturnObject::SetStatus(ReturnStatus status) {
+  m_status = status;
+  return Succeeded();
+}
 
 ReturnStatus CommandReturnObject::GetStatus() { return m_status; }
 
 bool CommandReturnObject::Succeeded() {
   return m_status <= eReturnStatusSuccessContinuingResult;
 }
 
 bool CommandReturnObject::HasResult() {