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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:120-127
+      // The setting value may have whitespace, double-quotes, or single-quotes
+      // around the file path to indicate that internal spaces are not word
+      // breaks.  Strip off any ws & quotes from the start and end of the file
+      // path - we aren't doing any word // breaking here so the quoting is
+      // unnecessary.  NB this will cause a problem if someone tries to specify
+      // a file path that legitimately begins or ends with a " or ' character,
+      // or whitespace.
----------------
This seems excessive. The command interpreter should already handle the word splitting and quotes, which means that things like `br set -y "f i l e.cpp":12:23` will work without this. And it can handle escaping properly, so `br set -y "\"file.cpp:12:23"` would work, if this code wouldn't get in the way.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:129
+      m_value_was_set = true;
+      m_file_spec.SetFile(file_name.str(), FileSpec::Style::native);
+      NotifyValueChanged();
----------------
`SetFile` already accepts a StringRef.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:74
+      
+      // First separate the file and the line specification:
+      llvm::StringRef right_of_last_colon;
----------------
jingham wrote:
> 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...
I am not a fan of regex parsing, but I still can't escape the feeling that this should be easier. Maybe a utility function would help:
```
bool chop_number(StringRef &str, uint32_t &num) {
  auto parts = str.rsplit(':');
  if (to_integer(parts.second, num)) {
    str = parts.first;
    return true;
  }
  return false;
}
...
if (!input.contains(':')
  one_colon_expected();
if (!chop_number(input, col))
  bad_line_number(); // This complains about line numbers because col will be promoted to a line number if the second chop_number fails.
if (!chop_number(input, line)) {
  line = col;
  col = 0;
}
file = input;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83975