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