[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 4 inline comments as done.
jingham 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.
----------------
labath wrote:
> 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.
I wondered about that too.  This is what OptionValueFileSpec did - I started this one from there.  So I removed it from both places, and the testsuite is still okay with it.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:74
+      
+      // First separate the file and the line specification:
+      llvm::StringRef right_of_last_colon;
----------------
labath wrote:
> 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;
> ```
The only tricky bit was making sure that you got the right error, and were able to show the string that was wrong.  I tried your approach but also passing around the found string bit, but it really didn't make anything clearer.

But I reworked it a little to make the logic clearer and added some comments.  This version seems pretty straightforward to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83975