[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

[Jonas Devlieghere via Phabricator via lldb-commits]

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