labath added a comment.
Overall, I am very supportive of this, though I have some comments inline. The
interface seems ok, though maybe you could add a couple of quick unit tests to
show how the class is meant to be used.
================
Comment at: lldb/source/Commands/CommandObjectBreakpoint.cpp:685
RegularExpression regexp(m_options.m_func_regexp);
- if (!regexp.IsValid()) {
- char err_str[1024];
- regexp.GetErrorAsCString(err_str, sizeof(err_str));
+ if (auto error = regexp.GetError()) {
result.AppendErrorWithFormat(
----------------
I wouldn't use `auto` in cases like these because `Optional<string>` is
definitely not among the things one would expect a function called `GetError`
to return. Though maybe that means the function should really return an
llvm::Error?
================
Comment at: lldb/source/Host/common/Socket.cpp:287
+ if (g_regex.Execute(host_and_port, &matches)) {
+ if (matches.size() >= 3) {
+ host_str = matches[1].str();
----------------
Are there any cases where evaluation of the regex can succeed, but the result
will not contain the expected number of matches? Should this be changed into an
assert, or just dropped completely?
================
Comment at: lldb/unittests/Utility/NameMatchesTest.cpp:52-53
EXPECT_TRUE(NameMatches("foobar", NameMatch::RegularExpression, "f[oa]o"));
- EXPECT_TRUE(NameMatches("foo", NameMatch::RegularExpression, ""));
- EXPECT_TRUE(NameMatches("", NameMatch::RegularExpression, ""));
+ EXPECT_FALSE(NameMatches("", NameMatch::RegularExpression, ""));
+ EXPECT_FALSE(NameMatches("foo", NameMatch::RegularExpression, ""));
EXPECT_FALSE(NameMatches("foo", NameMatch::RegularExpression, "b"));
----------------
This is interesting. So, llvm::regex considers an empty pattern to not match
anything? That doesn't sound right. I've just tried grep, python and perl, and
all of them seem perfectly happy to accept an empty string as a pattern which
matches everything..
I'd say this is a bug in llvm::regex we should fix.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66174/new/
https://reviews.llvm.org/D66174
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
