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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:86
 
+bool Watchpoint::SetupVariableWatchpointDisabler(StackFrameSP frame_sp) const {
+  ThreadSP thread_sp = frame_sp->GetThread();
----------------
Should you also verify that the `frame_sp` you got isn't `nullptr`? If it'll never be null, consider passing a `StackFrame &` instead.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:103-104
+  TargetSP target_sp = exe_ctx.GetTargetSP();
+  if (!target_sp)
+    return false;
+
----------------
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.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:129-133
+  if (!baton)
+    return false;
+
+  if (!context)
+    return false;
----------------
nit: You can merge this into one:
```
if (!baton || !context)
  return false;
```


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:140-141
+
+  LLDB_LOGF(log, "Watchpoint::%s called by breakpoint %" PRIu64 ".%" PRIu64,
+            __FUNCTION__, break_id, break_loc_id);
+
----------------
Any reason to not use `LLDB_LOG` here? You'll get `__FILE__` and `__FUNCTION__` included.


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:152
+  if (!process_sp)
+    return true;
+
----------------
This should be `return false;`


================
Comment at: lldb/source/Breakpoint/Watchpoint.cpp:165
+    process_sp->DisableWatchpoint(watch_sp.get());
+    return false;
+  }
----------------
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151366