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