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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Interpreter/OptionValueFileColonLine.h:1
+//===-- OptionValueFileColonLine.h -----------------------------------*- C++ -*-===//
+//
----------------
This actually looks like a legit issue.


================
Comment at: lldb/include/lldb/Interpreter/OptionValueFileColonLine.h:26
+
+  // Virtual subclass pure virtual overrides
+
----------------
I think these comments add very little, if anything. The `override` keyword already conveys the same information. Same for the comment below. 


================
Comment at: lldb/include/lldb/Interpreter/OptionValueFileColonLine.h:43
+    m_line_number = LLDB_INVALID_LINE_NUMBER;
+    m_column_number = 0;
+  }
----------------
This should be `LLDB_INVALID_COLUMN_NUMBER` and probably also be `UINT32_MAX` unless we rely on it being 0 anywhere?


================
Comment at: lldb/include/lldb/lldb-enumerations.h:540
   eArgTypeLineNum,
+  eArgTypeLineSpec,
   eArgTypeLogCategory,
----------------
`FileLineColumn` maybe to match the other enum value?


================
Comment at: lldb/source/Interpreter/OptionValue.cpp:474
     return "enum";
+  case eTypeFileColonLine:
+    return "file:line:column specifier";
----------------
Can we rename this to `eTypeFileLineColumn` to make it explicit that it can include all three? 


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:1
+//===-- OptionValueFileColonLine.cpp -------------------------------------------===//
+//
----------------
Can you please clang format this file? I know you don't (always) agree but otherwise there are going to be a bunch of unrelated changes when someone else touches (part of) this file. 


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:45
+      strm.PutCString(" = ");
+
+    if (m_file_spec)
----------------
spurious newline?


================
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
----------------
period


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:74
+      
+      // First separate the file and the line specification:
+      llvm::StringRef right_of_last_colon;
----------------
I think parsing this would be a lot easier with a regex: `([^:]+):(\d+)(:(\d+))?`. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83975