Code review changes -- update thread-id argument name, separate into separate group, and format

Author: mdko

lldb/source/Commands/CommandObjectThread.cpp

@@ -1134,22 +1134,25 @@ class CommandObjectThreadUntil : public CommandObjectParsed {
 
 class CommandObjectThreadSelect : public CommandObjectParsed {
 public:
-  class CommandOptions : public Options {
+  class OptionGroupThreadSelect : public OptionGroup {
   public:
-    CommandOptions() { OptionParsingStarting(nullptr); }
+    OptionGroupThreadSelect() { OptionParsingStarting(nullptr); }
 
-    ~CommandOptions() override = default;
+    ~OptionGroupThreadSelect() override = default;
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
-      m_thread_id = false;
+      m_thread_id = LLDB_INVALID_THREAD_ID;
     }
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                           ExecutionContext *execution_context) override {
-      const int short_option = m_getopt_table[option_idx].val;
+      const int short_option = g_thread_select_options[option_idx].short_option;
       switch (short_option) {
       case 't': {
-        m_thread_id = true;
+        if (option_arg.getAsInteger(0, m_thread_id)) {
+          m_thread_id = LLDB_INVALID_THREAD_ID;
+          return Status("Invalid thread ID: '%s'.", option_arg.str().c_str());
+        }
         break;
       }
 
@@ -1164,7 +1167,7 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
       return llvm::ArrayRef(g_thread_select_options);
     }
 
-    bool m_thread_id;
+    lldb::tid_t m_thread_id;
   };
 
   CommandObjectThreadSelect(CommandInterpreter &interpreter)
@@ -1179,13 +1182,17 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
     // Define the first (and only) variant of this arg.
     thread_idx_arg.arg_type = eArgTypeThreadIndex;
     thread_idx_arg.arg_repetition = eArgRepeatPlain;
+    thread_idx_arg.arg_opt_set_association = LLDB_OPT_SET_1;
 
     // There is only one variant this argument could be; put it into the
     // argument entry.
     arg.push_back(thread_idx_arg);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg);
+
+    m_option_group.Append(&m_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_2);
+    m_option_group.Finalize();
   }
 
   ~CommandObjectThreadSelect() override = default;
@@ -1201,48 +1208,55 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
         nullptr);
   }
 
-  Options *GetOptions() override { return &m_options; }
+  Options *GetOptions() override { return &m_option_group; }
 
 protected:
   void DoExecute(Args &command, CommandReturnObject &result) override {
     Process *process = m_exe_ctx.GetProcessPtr();
     if (process == nullptr) {
       result.AppendError("no process");
       return;
-    } else if (command.GetArgumentCount() != 1) {
+    } else if (m_options.m_thread_id == LLDB_INVALID_THREAD_ID && command.GetArgumentCount() != 1) {
       result.AppendErrorWithFormat(
-          "'%s' takes exactly one thread %s argument:\nUsage: %s\n",
-          m_cmd_name.c_str(), m_options.m_thread_id ? "ID" : "index",
-          m_cmd_syntax.c_str());
+          "'%s' takes exactly one thread index argument, or a thread ID option:\nUsage: %s\n",
+          m_cmd_name.c_str(), m_cmd_syntax.c_str());
       return;
-    }
-
-    uint32_t index_id;
-    if (!llvm::to_integer(command.GetArgumentAtIndex(0), index_id)) {
-      result.AppendErrorWithFormat("Invalid thread %s '%s'",
-                                   m_options.m_thread_id ? "ID" : "index",
-                                   command.GetArgumentAtIndex(0));
+    } else if (m_options.m_thread_id != LLDB_INVALID_THREAD_ID && command.GetArgumentCount() != 0) {
+      result.AppendErrorWithFormat(
+          "'%s' cannot take both a thread ID option and a thread index argument:\nUsage: %s\n",
+          m_cmd_name.c_str(), m_cmd_syntax.c_str());
       return;
     }
 
     Thread *new_thread = nullptr;
-    if (m_options.m_thread_id) {
-        new_thread = process->GetThreadList().FindThreadByID(index_id).get();
+    if (command.GetArgumentCount() == 1) {
+      uint32_t index_id;
+      if (!llvm::to_integer(command.GetArgumentAtIndex(0), index_id)) {
+        result.AppendErrorWithFormat("Invalid thread index '%s'",
+                                     command.GetArgumentAtIndex(0));
+        return;
+      }
+      new_thread = process->GetThreadList().FindThreadByIndexID(index_id).get();
+      if (new_thread == nullptr) {
+        result.AppendErrorWithFormat("Invalid thread #%s.\n",
+                                    command.GetArgumentAtIndex(0));
+        return;
+      }
     } else {
-        new_thread = process->GetThreadList().FindThreadByIndexID(index_id).get();
-    }
-    if (new_thread == nullptr) {
-      result.AppendErrorWithFormat("invalid thread %s%s.\n",
-                                   m_options.m_thread_id ? "ID " : "#",
-                                   command.GetArgumentAtIndex(0));
-      return;
+      new_thread = process->GetThreadList().FindThreadByID(m_options.m_thread_id).get();
+      if (new_thread == nullptr) {
+        result.AppendErrorWithFormat("Invalid thread ID %lu.\n",
+                                     m_options.m_thread_id);
+        return;
+      }
     }
 
     process->GetThreadList().SetSelectedThreadByID(new_thread->GetID(), true);
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
   }
 
-  CommandOptions m_options;
+  OptionGroupThreadSelect m_options;
+  OptionGroupOptions m_option_group;
 };
 
 // CommandObjectThreadList

lldb/source/Commands/Options.td

@@ -1118,8 +1118,8 @@ let Command = "thread plan list" in {
 }
 
 let Command = "thread select" in {
-  def thread_thread_id : Option<"thread_id", "t">,
-    Desc<"Provide a thread ID instead of a thread index.">;
+  def thread_select_thread_id : Option<"thread-id", "t">, Group<2>,
+    Arg<"ThreadID">, Desc<"Provide a thread ID instead of a thread index.">;
 }
 
 let Command = "thread trace dump function calls" in {

lldb/test/API/commands/thread/select/TestThreadSelect.py

@@ -20,18 +20,28 @@ def test_invalid_arg(self):
         self.expect(
             "thread select -t 0x1ffffffff",
             error=True,
-            startstr="error: Invalid thread ID '0x1ffffffff'",
+            startstr="error: Invalid thread ID",
+        )
+        self.expect(
+            "thread select 1 2 3",
+            error=True,
+            startstr="error: 'thread select' takes exactly one thread index argument, or a thread ID option:",
+        )
+        self.expect(
+            "thread select -t 1234 1",
+            error=True,
+            startstr="error: 'thread select' cannot take both a thread ID option and a thread index argument:",
         )
         # Parses but not a valid thread id.
         self.expect(
             "thread select 0xffffffff",
             error=True,
-            startstr="error: invalid thread #0xffffffff.",
+            startstr="error: Invalid thread #0xffffffff.",
         )
         self.expect(
             "thread select -t 0xffffffff",
             error=True,
-            startstr="error: invalid thread ID 0xffffffff.",
+            startstr="error: Invalid thread ID",
         )
 
     def test_thread_select_tid(self):