[Lldb-commits] [PATCH] D97076: [lldb] Refine ThreadPlan::ShouldAutoContinue

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/include/lldb/Target/ThreadPlan.h:354-359
+  /// Returns whether this thread plan overrides the `ShouldStop` of previous
+  /// plans.
+  ///
+  /// When processing the thread plan stack, this function gives plans the
+  /// ability to continue - even when previous plans subsequently return false
+  /// from `ShouldStop`. \see Thread::ShouldStop
----------------
kastiglione wrote:
> jingham wrote:
> > kastiglione wrote:
> > > Happy to iterate on this. I wanted to take the opportunity to explain this, since by name it may seem unclear how exactly it relates to `ShouldStop`.
> > The use of "previous plans" is a little confusing, since it not clear "previous to what".   To me the obvious meaning is plans previously dealt with in this negotiation, which is not what you mean, and then sounds really odd when you hit the "subsequently".  I don't think you need the "previous" at all.  You could just say "subsequently processed plans".
> Thanks I'll update it.
> 
> I agree that "previous" can be ambiguous, especially in the context of `Thread::ShouldStop`. What do you think of renaming `GetPreviousPlan` to `GetParentPlan` (and related variables and documentation too)? I can do this in a follow up if you agree.
Changing away from previous would be great! 

Parent isn't quite right anyway because there's no necessary relationship between plans on the stack.  You could stop at a breakpoint while using one plan, and then do a "step" and the plan that was working before the breakpoint has no knowledge or relationship to the new plan.

I've been trying recently to be consistent about using Older and Younger for things pushed onto a stack. That seems clear to me since you are describing the process of pushing things onto a stack, so the order in the stack is governed by the time at which they were pushed.  I've been trying to speak of stack frames this way as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97076