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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I think we need to fix the behavior for "", other than that, this looks fine to me.



================
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();
----------------
labath wrote:
> 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?
Jonas introduced this pattern everywhere he calls Execute.  It does seem a little odd, since there's no way any of the patterns that he checks matches for could match but not match the () part.  It makes the reader wonder if they don't understand regular expressions after all...


================
Comment at: lldb/source/Target/ThreadPlanStepInRange.cpp:367
+            avoid_regexp_to_use->Execute(frame_function_name, &matches);
+        Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
+        if (return_value && log && matches.size() >= 2) {
----------------
You can move this into the "if (return_value"  It doesn't get used outside the check, and this test mostly fails so it would be good not to do more work in that case.


================
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"));
----------------
labath wrote:
> 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.
Yes, that does seem odd to me.  I would expect:

(lldb) break set -r ""

to match everything, not nothing.


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

https://reviews.llvm.org/D66174