JDevlieghere added inline comments.
================ Comment at: lldb/include/lldb/Breakpoint/BreakpointLocation.h:70 - /// If \a enable is \b true, enable the breakpoint, if \b false disable it. + /// If \a enabled is \b true, enable the breakpoint, if \b false disable it. void SetEnabled(bool enabled); ---------------- I recommend splitting of these unrelated changes into a separate NFC patch. ================ Comment at: lldb/include/lldb/Breakpoint/BreakpointLocation.h:138 + /// If condition is injected \b true, \b false otherwise. + bool GetInjectCondition(void) const; + ---------------- Remove the void argument. ================ Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:410 // condition is updated. + bool m_inject_condition; // If set, inject breakpoint condition + // into process. ---------------- This should be a `///` above the variable. If you want you can fix up the other comments here as well in that NFC commit I proposed earlier. ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:41 + def test_injected_breakpoint_condition_flag_api(self): + """Exercise fast conditional breakpoint with SB API""" + self.build() ---------------- `s/breakpoint with SB API/breakpoints with the SB API/` ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:45 + + def enable_injected_breakpoint_condition(self, cli): + exe = self.getBuildArtifact(self.binary) ---------------- What does `cli` stand for? ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:51 + if cli == True: + lldbutil.run_break_set_by_source_regexp( + self, self.comment, self.extra_options ---------------- Did you copy this code style from somewhere? If so it's fine, but otherwise I'd stick to the common style. I think the "great refactor" commit includes the command used to format the tests. I use `yapf` for Python but it doesn't exactly match. ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:59 + else: + # now create a breakpoint on main.c by source regex'. + breakpoint = self.target.BreakpointCreateBySourceRegex( ---------------- Sentences should start with an uppercase character. Same below. ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:63 + ) + # print("breakpoint:", breakpoint) + self.assertTrue( ---------------- Remove commented out code. ================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/main.c:1 +//===-- main.c --------------------------------------------------*- C++ -*-===// +// ---------------- I think the consensus is that we don't need the header in test files. ================ Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:116 -BREAKPOINT_HIT_ONCE = "Breakpoint resolved with hit cout = 1" +BREAKPOINT_HIT_ONCE = "Breakpoint resolved with hit count = 1" ---------------- NFC commit ================ Comment at: lldb/source/API/SBBreakpoint.cpp:316 +void SBBreakpoint::SetInjectCondition(bool inject_condition) { + LLDB_RECORD_METHOD(void, SBBreakpoint, SetInjectCondition, (bool), + inject_condition); ---------------- Did you register these function wiht `LLDB_REGISTER_METHOD`? You should be able to just run `lldb-inst` over the file to have them generated. ================ Comment at: lldb/source/API/SBBreakpoint.cpp:320 + BreakpointSP bkpt_sp = GetSP(); + if (bkpt_sp) { + std::lock_guard<std::recursive_mutex> guard( ---------------- I'd invert the condition and make this an early return. ================ Comment at: lldb/source/API/SBBreakpoint.cpp:331 + BreakpointSP bkpt_sp = GetSP(); + if (bkpt_sp) { + std::lock_guard<std::recursive_mutex> guard( ---------------- I'd invert the condition and make this an early return. ================ Comment at: lldb/source/Breakpoint/BreakpointLocation.cpp:233 + return m_options_up->GetInjectCondition(); + else + return m_owner.GetInjectCondition(); ---------------- No `else` after return. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66248/new/ https://reviews.llvm.org/D66248 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits