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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to