[Lldb-commits] [PATCH] D83975: Add an option to "break set" and "source list" that takes a line spec in the form file:line:column

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 8 inline comments as done.
jingham added inline comments.


================
Comment at: lldb/include/lldb/Interpreter/OptionValueFileColonLine.h:26
+
+  // Virtual subclass pure virtual overrides
+
----------------
JDevlieghere wrote:
> I think these comments add very little, if anything. The `override` keyword already conveys the same information. Same for the comment below. 
I removed them here, but note they were just copied from OptionValueFileSpec.h.  We started with some header file template that had these way back in the day, and they are all over.  If we don't want them around then it would be better to just do that as a patch and not piecemeal.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:67
+    if (value.size() > 0) {
+      // This is in the form filename:linenumber:column
+      // I wish we could use filename:linenumber.column, that would make the
----------------
JDevlieghere wrote:
> period
Ah, nuts.  I forgot the period.  I'll add it before checking this in.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:74
+      
+      // First separate the file and the line specification:
+      llvm::StringRef right_of_last_colon;
----------------
JDevlieghere wrote:
> I think parsing this would be a lot easier with a regex: `([^:]+):(\d+)(:(\d+))?`. What do you think?
A regex seems overpowered for what I'm doing here, plus that might tempt people to add more stuff to the specifier and I don't want to back into -y being the gdb breakpoint specifier mini-language...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83975/new/

https://reviews.llvm.org/D83975