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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
