jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
This change didn't immediately make sense to me, which means it's a bit tricky
and tricky changes should get comments or they will confuse us later on. Plus,
I wasn't sure how it worked, so I'm looking forward to reading the comment as
part of the review...
================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2005
// stored "repeat command" which we should give a chance to produce it's
// repeat command, even though we don't add repeat commands to the history.
+ Args command_args(command_string);
----------------
You at least need to fix this comment, since you aren't checking empty_command
anymore.
It would also be good to explain the logic here, since I'm not entirely sure
why this patch fixes the problem. It should just mean that we are setting the
"previous repeat command" twice, once above and once here, in the case where we
didn't set "empty_command", so it's not clear why that helps.
I also think it's wrong to set a repeat command when add_to_history is false.
That generally happens if some code, like a breakpoint command, uses
HandleCommand, i.e. that's a command the user didn't directly execute. So it
doesn't make sense that that non-user command should set the user repeat
command.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143695/new/
https://reviews.llvm.org/D143695
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
