[Lldb-commits] [PATCH] D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

A few more nits as I was reading through the code, but this generally LGTM.



================
Comment at: lldb/source/Commands/CommandCompletions.cpp:811-813
+  sort(to_delete.begin(), to_delete.end(), std::greater<size_t>());
+  for (size_t idx : to_delete)
+    args.DeleteArgumentAtIndex(idx);
----------------
jingham wrote:
> JDevlieghere wrote:
> > I'm surprised this works. Don't the indexes shift when one's deleted? I assumed that's why you're sorting them. 
> Why wouldn't it?
> 
> The delete starts with the greatest index, so each time you delete an entry it shuffles the ones above it down, but it doesn't reshuffle the ones below it.  Since all the remaining indices are lower than the one you just deleted, they are all still pointing to the elements you intend to remove.
Got it, I got the direction wrong.


================
Comment at: lldb/source/Commands/CommandObjectCommands.cpp:2028
+    if (num_args == 0) {
+      result.AppendError("No command was specified.");
+      return false;
----------------
This method still has some inconsistencies in capitalization. 


================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:34-35
+
+  CommandObject::CommandMap::iterator pos;
+  pos = m_subcommand_dict.find(std::string(sub_cmd));
+  if (pos == m_subcommand_dict.end())
----------------



================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:54-67
   CommandObject::CommandMap::iterator pos;
 
-  if (!m_subcommand_dict.empty()) {
+  StringList local_matches;
+  if (matches == nullptr)
+    matches = &local_matches;
+  int num_matches =
+      AddNamesMatchingPartialString(m_subcommand_dict, sub_cmd, *matches);
----------------



================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:112-115
+  CommandMap::iterator pos;
+  std::string str_name(name);
+
+  pos = m_subcommand_dict.find(str_name);
----------------



================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:121-129
+  const char *error_str = nullptr;
+  if (!can_replace)
+    error_str = "sub-command already exists";
+  if (!(*pos).second->IsUserCommand())
+    error_str = "can't replace a builtin subcommand";
+
+  if (error_str) {
----------------
This inline diff looks more confusing than anything, but basically I just switched the errors around (instead of the current fall-through) and return the error immediately. 


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1259-1262
+  StringList *matches_ptr = matches;
+  StringList tmp_list;
+  if (!matches_ptr)
+    matches_ptr = &tmp_list;
----------------



================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1369-1370
+bool CommandInterpreter::RemoveUserMultiword(llvm::StringRef multi_name) {
+  CommandObject::CommandMap::iterator pos =
+      m_user_mw_dict.find(std::string(multi_name));
+  if (pos != m_user_mw_dict.end()) {
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110298/new/

https://reviews.llvm.org/D110298