JDevlieghere added inline comments.
================ Comment at: lldb/source/Breakpoint/Watchpoint.cpp:127 + +bool Watchpoint::VariableWatchpointDisabler(void *baton, + StoppointCallbackContext *context, ---------------- I think this should return an `llvm::Error` and log it at the call site. For every early return we know what went wrong. Same for `SetupVariableWatchpointDisabler`. ================ Comment at: lldb/source/Breakpoint/Watchpoint.cpp:140 + + LLDB_LOGF(log, "Watchpoint::%s called by breakpoint %" PRIu64 ".%" PRIu64, + __FUNCTION__, break_id, break_loc_id); ---------------- Doesn't __FUNCTION__ already include `Watchpoint::`? Also I think there's a flag to include the function name in the log, so this is redundant. ================ Comment at: lldb/source/Commands/CommandObjectWatchpoint.cpp:959 ", variable expression='%s').\n", addr, (uint64_t)size, command.GetArgumentAtIndex(0)); if (error.AsCString(nullptr)) ---------------- You wouldn't need the `(uint64_t)` cast if you used formatv. This also should use a C++ style cast (https://discourse.llvm.org/t/rfc-add-preferred-casting-style-to-coding-standards/70793/4). ================ Comment at: lldb/source/Commands/CommandObjectWatchpoint.cpp:960-961 addr, (uint64_t)size, command.GetArgumentAtIndex(0)); if (error.AsCString(nullptr)) result.AppendError(error.AsCString()); + return result.Succeeded(); ---------------- `Status::AsCString` isn't trivial so probably worth calling this one and storing it in a temporary variable. 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