[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.

I did a quick pass as I was reading through the patch to understand it. I'll do another one tomorrow.



================
Comment at: lldb/include/lldb/Interpreter/CommandCompletions.h:156-159
+  // This completer works for commands whose only arguments are a command path.
+  // It isn't tied to an argument type because it completes not on a single
+  // argument but on the sequence of arguments, so you have to invoke it by
+  // hand.
----------------



================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:238
+    eCommandTypesAliases = 0x0008, // aliases such as "po"
+    eCommandTypesHidden  = 0x0010,  // commands prefixed with an underscore
     eCommandTypesAllThem = 0xFFFF  // all commands
----------------
Really these should all be Doxygen comments (`//<`) but I know you're just adding values. 


================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:309-311
+  CommandObjectMultiword *VerifyUserMultiwordCmdPath(Args &path,
+                                                 bool leaf_is_command,
+                                                 Status &result);
----------------
Please clang-format before landing. 


================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:324
 
+  bool RemoveUserMultiword(llvm::StringRef alias_name);
+
----------------
Copy/paste? 


================
Comment at: lldb/include/lldb/Interpreter/CommandObject.h:198
+    Status result;
+    result.SetErrorString("can't add a subcommand to a plain command");
+    return result;
----------------
Maybe say "built-in" command or "non user-defined" command? 


================
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);
----------------
I'm surprised this works. Don't the indexes shift when one's deleted? I assumed that's why you're sorting them. 


================
Comment at: lldb/source/Commands/CommandObjectCommands.cpp:1543
+    if (path_error.Fail()) {
+      result.AppendErrorWithFormat("Error in command path: %s",
+                                   path_error.AsCString());
----------------
This should probably be lowercase to be consistent with the other error strings. 


================
Comment at: lldb/source/Commands/CommandObjectCommands.cpp:1710
+        !m_interpreter.HasUserMultiwordCommands()) {
+      result.AppendErrorWithFormat("Can only delete user defined commands, "
+                                   "but no user defined commands found");
----------------
Lowercase?


================
Comment at: lldb/source/Commands/CommandObjectCommands.cpp:1899
+    if (num_args == 0) {
+      result.AppendError("No command was specified.");
+      return false;
----------------
All the previous errors started with a lowercase letter. I don't have a preference, but we should be consistent.


================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:99
 
+Status CommandObjectMultiword::LoadUserSubcommand(
+    llvm::StringRef name, const CommandObjectSP &cmd_obj, bool can_replace) {
----------------
In line with the direction of preferring `llvm::Error` over `lldb_private::Status`, I think these two functions would be good candidates to return `llvm::Error`s instead. We can always convert them to a Status up the chain. 


================
Comment at: lldb/source/Commands/CommandObjectMultiword.cpp:102-104
+  if (cmd_obj)
+    assert((&GetCommandInterpreter() == &cmd_obj->GetCommandInterpreter()) &&
+           "tried to add a CommandObject from a different interpreter");
----------------
This is not going to compile in a non-assert build. You'll want to inline the if-check in the assert.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:900
 
+CommandObjectMultiword *CommandInterpreter::VerifyUserMultiwordCmdPath(
+    Args &path, bool leaf_is_command, Status &result) {
----------------
Similar to my previous comment, this could be an expected instead of using a Status as an output argument. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110298