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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits