jingham added a comment. This looks good to me, except that instead of leaving all the other variants of AddCommand, they should funnel through the one that takes the most arguments. It was poor form to leave two around and more so now that there's three.
I'm beginning to think we need an SBAddCommandOptions or something like that so we don't need to add yet another variant next time we want to add some flags to a command. That is a nice-to-have, however. If you are done with this change, it's okay as it stands. In retrospect, I think it would be better if the CommandInterpreter were to prepend the container commands to the repeat command string. It is bogus that multi-word commands have to know their hierarchy to emit a useful repeat command string. Prepending the containing commands would mean that command A couldn't invoke a totally different command as its repeat command. But that seems like an odd thing to do anyway. And that way a command wouldn't have to know where it is situated in the command hierarchy for the repeat command to work. That makes your test ugly, but that's not your fault (it's probably some earlier version of me's decision). ================ Comment at: lldb/source/API/SBCommandInterpreter.cpp:857 +lldb::SBCommand SBCommand::AddCommand(const char *name, + lldb::SBCommandPluginInterface *impl, ---------------- Instead of keeping these copies of AddCommand around, can you funnel all of them through the one that takes the auto_repeat command? That would reduce boilerplate and make adding the next thing easier... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77444/new/ https://reviews.llvm.org/D77444 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
