jingham added a comment.

This seems like a fine improvement.  One little nit, I would ask the current 
plan ShouldAutoContinue before popping it.  Popping the plan does call WillPop, 
so the plan does have a chance to react to being popped, and you don't know 
what it will do.  So to be on the safe side it would be better to ask questions 
of it as an active plan before you pop it.



================
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:
> 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".


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