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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
