[Lldb-commits] [PATCH] D70002: [lldb] Make Target* a Target& in CommandObjectExpression::DoExecute REPL logic

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't think either version is right.

We really shouldn't be using "GetSelectedTarget" (with or without the Dummy) for getting the Target context for a command.  That's how it was originally done, but you aren't guaranteed that the selected target is the right target context in callbacks that use commands.

For instance, if lldb has two targets running in one debugger, and Target A is the currently selected target, but target B hits a breakpoint and the breakpoint has commands, we need to make sure the commands run in the breakpoint use target B.  But we don't select Target B when we hit the breakpoint, since I want to keep that Selected Target more under the user's control.  Instead we set the appropriate target in the command's m_exe_ctx, and commands are supposed to pull it from there.  BTW, I see that there are still a bunch of commands that haven't been updated to use this.  Maybe we ought to have a "GetCurrentOrDummyTarget" in CommandObject that returns the target from m_exe_ctx, or the dummy target if that is empty.  Either that or put the Dummy target IN m_exe_ctx if there is no appropriate target, and have everybody use that.

Anyway, this isn't more wrong than the previous version, but it isn't right either...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70002/new/

https://reviews.llvm.org/D70002