[PATCH] D68054: Regex: Add static convenience functions for "match" and "sub"

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: include/llvm/Support/Regex.h:90-92
+    /// Assuming no regex compilation errors, equivalent to the following:
+    ///
+    ///   Regex(RegexPattern, Flags, Error).match(String, Matches, Error)
----------------
While redundant, I'd add "which returns an error if the regex is invalid or no match is found" to be more explicit


================
Comment at: include/llvm/Support/Regex.h:115-117
+    /// Assuming no regex compilation errors, equivalent to the following:
+    ///
+    ///   Regex(RegexPattern, Flags, Error).sub(Repl, String, Error)
----------------
Same as above.


================
Comment at: tools/sancov/sancov.cpp:127
+static const Regex SancovFileRegex("(.*)\\.[0-9]+\\.sancov");
+static const Regex SymcovFileRegex(".*\\.symcov");
 
----------------
nlguillemot wrote:
> Missed making these ones `const` in the previous commit, so updated them in this one.
I know it's a small change but I'd rather you split it out in a separate patch which I'll approve gladly. This allow easier revert or cherry-pick.


================
Comment at: unittests/Support/RegexTest.cpp:20
 
+using RegexDeathTest = RegexTest;
+
----------------
nlguillemot wrote:
> This typedef might seem kind of redundant, but it's recommended by the googletest guide:
> 
> https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#death-test-naming
Thanks for bringing this to my attention, I was not aware of it.


================
Comment at: unittests/Support/RegexTest.cpp:200-203
+  EXPECT_TRUE(Regex::match("^[0-9]+$", "916", nullptr, Regex::NoFlags, &Error));
+  EXPECT_TRUE(Error.empty());
+  EXPECT_FALSE(Regex::match("(foo", "foo", nullptr, Regex::NoFlags, &Error));
+  EXPECT_EQ("parentheses not balanced", Error);
----------------
I'd exchange the two tests like you did in the Constructor test because it shows that calling the wrapper on a valid regex will clear Error while here it only shows it does not set it.


================
Comment at: unittests/Support/RegexTest.cpp:206-209
+  EXPECT_EQ("NUM", Regex::sub("[0-9]+", "NUM", "1234", Regex::NoFlags, &Error));
+  EXPECT_TRUE(Error.empty());
+  EXPECT_EQ("aber", Regex::sub("a[b-", "d", "aber", Regex::NoFlags, &Error));
+  EXPECT_EQ("invalid character range", Error);
----------------
Likewise.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68054