[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 marked an inline comment as done.
labath added a comment.

Looks fine to me, modulo the clang-format issues.



================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:74
+      
+      // First separate the file and the line specification:
+      llvm::StringRef right_of_last_colon;
----------------
jingham wrote:
> 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.
Yeah, this looks better. I see now the reason for some of that complexity -- you wanted `foo:2b:47` to be considered invalid instead of as "line 47 in file `foo:2b` -- which seems reasonable.


================
Comment at: lldb/source/Interpreter/OptionValueFileColonLine.cpp:96
+      if (middle_piece.empty() || middle_piece.getAsInteger(0, m_line_number)) {
+        // getAsInteger returns true when it fails, so there were only two 
+        // legit pieces; our original division was right.  Reassign the file
----------------
This is one of the reasons I'd recommend `llvm::to_integer` over `StringRef::getAsInteger`.


================
Comment at: lldb/unittests/Interpreter/TestOptionValueFileColonLine.cpp:37
+TEST(OptionValueFileColonLine, setFromString) {
+  bool success = false;
+  OptionValueFileColonLine value;
----------------
unused variable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83975