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

[Jim Ingham via Phabricator via lldb-commits]

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