[Lldb-commits] [PATCH] D51461: Support setting a breakpoint bile FileSpec+Line+Column in the SBAPI.

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This looks great, just a few small nits and a test request.



================
Comment at: include/lldb/Breakpoint/BreakpointResolverFileLine.h:71
+  uint32_t m_column; ///< This is the column that we are looking for.
+  /// This determines whether the resolver looks for inlined functions or not.
+  bool m_inlines;
----------------
This comment should go after m_inlines and get a ///<


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py:23-31
+        exe = self.getBuildArtifact("a.out")
+        target = self.dbg.CreateTarget(exe)
+        breakpoint = target.BreakpointCreateByLocation(
+            lldb.SBFileSpec("main.c"), 20, 50, 0, lldb.SBFileSpecList())
+
+        process = target.LaunchSimple(
+            None, None, self.get_process_working_directory())
----------------
Can you add a run_to_line_breakpoint to lldbutil.py and use it here?  Should just take a couple of lines, (see run_to_source_breakpoint and run_to_name_breakpoint) and I think it's better to centralize this logic in lldbutil.


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py:42-50
+        exe = self.getBuildArtifact("a.out")
+        target = self.dbg.CreateTarget(exe)
+        breakpoint = target.BreakpointCreateByLocation(
+            lldb.SBFileSpec("main.c"), 20, 0, 0, lldb.SBFileSpecList())
+
+        process = target.LaunchSimple(
+            None, None, self.get_process_working_directory())
----------------
Then use it here too...


================
Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:67-72
+  success =
+      options_dict.GetValueForKeyAsInteger(GetKey(OptionNames::Column), column);
+  if (!success) {
+    error.SetErrorString("BRFL::CFSD: Couldn't find column number entry.");
+    return nullptr;
+  }
----------------
Should probably make this optional or it will break old save files.  If it's not there set the column to 0.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51461