JDevlieghere added inline comments.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:270-271
+Status ScriptedProcess::EnableBreakpointSite(BreakpointSite *bp_site) {
+ assert(bp_site != nullptr);
+
----------------
bulbazord wrote:
> I like the assert, but I can't help but wonder if we should change the
> signature of this method? Something like `EnableBreakpointSite(BreakpointSite
> &bp_site)` or something like this... Seems like the other overrides assume
> that it is not nullptr before using it and all the callsites verify it before
> calling it too.
+1. I wondered the same thing when looking at the header.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:281
+
+ return EnableSoftwareBreakpoint(bp_site);
+}
----------------
The same applies to `Process::EnableSoftwareBreakpoint` as well. It also has an
assert and seems like it should just be checked in the caller and enforced by
using a reference.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:75
+ Status EnableBreakpointSite(BreakpointSite *bp_site) override;
+
----------------
Should this take a reference instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145296/new/
https://reviews.llvm.org/D145296
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
