[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

[Pavel Labath via Phabricator via lldb-commits]

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