[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

[Jim Ingham via Phabricator via lldb-commits]

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