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

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 9 inline comments as done.
jingham added a comment.

Addresses Jonas' first round of comments.



================
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);
----------------
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.


================
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");
----------------
JDevlieghere wrote:
> This is not going to compile in a non-assert build. You'll want to inline the if-check in the assert.
The other places this check is done use lldbassert (in CommandInterpreter.h).  I'll switch to that for consistency's sake.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:900
 
+CommandObjectMultiword *CommandInterpreter::VerifyUserMultiwordCmdPath(
+    Args &path, bool leaf_is_command, Status &result) {
----------------
JDevlieghere wrote:
> Similar to my previous comment, this could be an expected instead of using a Status as an output argument. 
This one can't be an expected.  It returns nullptr in two cases:

1) When the path was wrong, in which case there was an error
2) When the path is a top-level multiword, in which case there's no error

This is documented in its help.

We wouldn't have to do it this way if we had made a "root level" multiword command that we add all the top-level commands to, because then the top level commands would have a container, but we didn't and they don't.  So I have to do it this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110298