[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

Resetting the hit counts on rerun is a more useful behavior, so this is all fine.  But the Target is the one that does all the breakpoint management, so I think the resetting should go through the Target, not the Process.  And we generally delegate "do on all breakpoints" operations to the BreakpointList, so it would be more consistent with the current structure to go Process::WillLaunch -> Target::ResetHitCounts -> BreakpointList::ResetHitCounts.



================
Comment at: lldb/source/Target/Process.cpp:2763-2766
+static void ResetHitCounts(Process &Proc) {
+  for (const auto &BP : Proc.GetTarget().GetBreakpointList().Breakpoints())
+    BP->ResetHitCount();
+}
----------------
fdeazeve wrote:
> JDevlieghere wrote:
> > Can this be a private member so we don't have to pass in `*this`? 
> No strong preferences on my part, but I had made it a free function because it can be implemented in terms of the public behaviour, i.e. it doesn't rely on implementation details.
Resetting all the hit counts in a BreakpointList seems to me a job of the BreakpointList.  Also, while DoWillLaunch/Attach is where this action belongs, which is how the Process gets involved, the Target is the one that manages the breakpoints in general.  So I think it would be better to have the Target do ResetHitCounts, and these Process calls should just dial up it's Target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858