[Lldb-commits] [PATCH] D151366: [lldb] Disable variable watchpoints when going out of scope

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:103-104
+  TargetSP target_sp = exe_ctx.GetTargetSP();
+  if (!target_sp)
+    return false;
+
----------------
bulbazord wrote:
> Is it even possible to have an `ExecutionContext` created from a `StackFrame` that doesn't have a valid `Target`? I wonder if we should assert that we got one here if not.
There is no `ExecutionContext::IsValid` method, so if you created an `ExecutionContext` with a bogus frame, you might get a nullptr.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:165
+    process_sp->DisableWatchpoint(watch_sp.get());
+    return false;
+  }
----------------
bulbazord wrote:
> This should be `return true;` no? You could invert the condition to be consistent with the patterns above of "return false if some condition wasn't met".
We want to always `return false` since this is a breakpoint callback, we never want to stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151366