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