Regex: Make "match" and "sub" const member functions

[nlguillemot]

Summary

The Regex "match" and "sub" member functions were previously not "const"
because they wrote to the "error" member variable. This commit removes those
assignments, and instead assumes that the validity of the regex is already
known after the initial compilation of the regular expression. As a result, these
member functions were possible to make "const". This makes it easier to do things like
pre-compile Regexes up-front, and makes "match" and "sub" thread-safe.
The error status is now returned as an optional output, which also makes the API of
"match" and "sub" more consistent with each other.

Also, some uses of Regex that could be refactored to be const were made const.

[thopre]

include/llvm/Support/Regex.h

76–77 ↗

(On Diff #218988)

Could you mention that any error will be cleared by the function?

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

I think I'd prefer the API to enforce the user to check compile error when creating the regex and just have an assert here. match should only report errors in matching IMHO.

[nlguillemot]

include/llvm/Support/Regex.h

76–77 ↗

(On Diff #218988)

The error is cleared in both match and sub, so I'll update the comment for both of those.

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

I completely agree in principle, though right now that would cause some API breakage.
For example, the following usage in lib/Transforms/Utils/SymbolRewriter.cpp:

std::string Name = Regex(Pattern).sub(Transform, C.getName(), &Error);
if (!Error.empty())
  report_fatal_error("unable to transforn " + C.getName() + " in " +
                     M.getModuleIdentifier() + ": " + Error);

I guess we could make the change anyways and update all the call sites. Maybe good for a follow-up patch?

[nlguillemot]

Added note in the comments for match and sub to explicitly note that the Error string is cleared when there is no error.

[thopre]

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

Yes, doing it in a separate would be nice. Ideally update call sites in a first patch and then apply this patch on top with those asserts I suggested.

[nlguillemot]

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

I think it would be nice if there was an alternative way to cleanly implement the "Create a Regex and immediately throw it away" idiom as used in the example in SymbolRewriter.cpp above.

For example, Python's API can be used both ways:

import re
# "one-shot" regex
re.match(some_pattern, some_string)
# pre-compiled regex
compiled = re.compile(some_pattern)
compiled.match(some_string)

Maybe we should have something similar, where the whole process of creating the Regex object and throwing it away is wrapped in one function call. It could maybe be implemented by static member functions where the first parameter is the regex pattern. That would also make the refactoring more easy for cases like the above.

By the way, I don't have commit access to LLVM yet. If the patch looks good to you so far, could you commit it on my behalf?

[thopre]

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

Indeed, sounds like a good idea. Yes I can commit the patch for you once it is approved.

Best regards.

[nlguillemot]

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

Thanks for helping me with the commit. Is it ok if I switch around the reviewers to make you the reviewer? There was no particular reason for the choice of the initial reviewer list other than the recent activity in the file, so I think your review feedback is probably enough.

[thopre]

lib/Support/Regex.cpp

88–90 ↗

(On Diff #218988)

That's usually the right way to go about it since someone who did a change in an area is likely to be interested in what's going on in that area in the future. You can add me to the reviewer list yes, but no need to remove anyone I think.

[nlguillemot]

friendly reminder ping

[thopre]

In D67241#1679348, @nlguillemot wrote:

friendly reminder ping

My bad, I misunderstood you and thought you'd do a patch to implement the "Create a Regex and immediately throw it away" idiom and rebase that one on top of it.

[nlguillemot]

In D67241#1679847, @thopre wrote:

In D67241#1679348, @nlguillemot wrote:

friendly reminder ping

My bad, I misunderstood you and thought you'd do a patch to implement the "Create a Regex and immediately throw it away" idiom and rebase that one on top of it.

I could do that. By "rebase that one on top of it", do you mean to include the additional changes in this same review?

[thopre]

In D67241#1679937, @nlguillemot wrote:

In D67241#1679847, @thopre wrote:

In D67241#1679348, @nlguillemot wrote:

friendly reminder ping

My bad, I misunderstood you and thought you'd do a patch to implement the "Create a Regex and immediately throw it away" idiom and rebase that one on top of it.

I could do that. By "rebase that one on top of it", do you mean to include the additional changes in this same review?

I meant make a patch series/stack where the first one adds the new idiom and the second is this one where the matching only checks for matching errors. But it's ok to do it the other way around, I'll commit this now once I've finished a build+check-all and you can add the changes for the new idiom on top of it.

Best regards.

llvm/trunk/include/llvm/Support/Regex.h

public:
     Regex &operator=(Regex regex) {
       std::swap(preg, regex.preg);
       std::swap(error, regex.error);
       return *this;
     }
     Regex(Regex &&regex);
     ~Regex();
 
-    /// isValid - returns the error encountered during regex compilation, or
-    /// matching, if any.
+    /// isValid - returns the error encountered during regex compilation, if
+    /// any.
     bool isValid(std::string &Error) const;
     bool isValid() const { return !error; }
 
     /// getNumMatches - In a valid regex, return the number of parenthesized
     /// matches it contains.  The number filled in by match will include this
     /// many entries plus one for the whole regex (as element 0).
     unsigned getNumMatches() const;
 
     /// matches - Match the regex against a given \p String.
     ///
     /// \param Matches - If given, on a successful match this will be filled in
     /// with references to the matched group expressions (inside \p String),
     /// the first group is always the entire pattern.
     ///
+    /// \param Error - If non-null, any errors in the matching will be recorded
+    /// as a non-empty string. If there is no error, it will be an empty string.
+    ///
     /// This returns true on a successful match.
-    bool match(StringRef String, SmallVectorImpl<StringRef> *Matches = nullptr);
+    bool match(StringRef String, SmallVectorImpl<StringRef> *Matches = nullptr,
+               std::string *Error = nullptr) const;
 
     /// sub - Return the result of replacing the first match of the regex in
     /// \p String with the \p Repl string. Backreferences like "\0" in the
     /// replacement string are replaced with the appropriate match substring.
     ///
     /// Note that the replacement string has backslash escaping performed on
     /// it. Invalid backreferences are ignored (replaced by empty strings).
     ///
     /// \param Error If non-null, any errors in the substitution (invalid
     /// backreferences, trailing backslashes) will be recorded as a non-empty
-    /// string.
+    /// string. If there is no error, it will be an empty string.
     std::string sub(StringRef Repl, StringRef String,
-                    std::string *Error = nullptr);
+                    std::string *Error = nullptr) const;
 
     /// If this function returns true, ^Str$ is an extended regular
     /// expression that matches Str and only Str.
     static bool isLiteralERE(StringRef Str);
 
     /// Turn String into a regex by escaping its special characters.
     static std::string escape(StringRef String);
 
   private:
     struct llvm_regex *preg;
     int error;
   };
 }
 
 #endif // LLVM_SUPPORT_REGEX_H

llvm/trunk/lib/IR/AutoUpgrade.cpp

if (Name.startswith("arm.neon.vclz")) {
                                "llvm.ctlz." + Name.substr(14), F->getParent());
       return true;
     }
     if (Name.startswith("arm.neon.vcnt")) {
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::ctpop,
                                         F->arg_begin()->getType());
       return true;
     }
-    Regex vldRegex("^arm\\.neon\\.vld([1234]|[234]lane)\\.v[a-z0-9]*$");
+    static const Regex vldRegex("^arm\\.neon\\.vld([1234]|[234]lane)\\.v[a-z0-9]*$");
     if (vldRegex.match(Name)) {
       auto fArgs = F->getFunctionType()->params();
       SmallVector<Type *, 4> Tys(fArgs.begin(), fArgs.end());
       // Can't use Intrinsic::getDeclaration here as the return types might
       // then only be structurally equal.
       FunctionType* fType = FunctionType::get(F->getReturnType(), Tys, false);
       NewFn = Function::Create(fType, F->getLinkage(), F->getAddressSpace(),
                                "llvm." + Name + ".p0i8", F->getParent());
       return true;
     }
-    Regex vstRegex("^arm\\.neon\\.vst([1234]|[234]lane)\\.v[a-z0-9]*$");
+    static const Regex vstRegex("^arm\\.neon\\.vst([1234]|[234]lane)\\.v[a-z0-9]*$");
     if (vstRegex.match(Name)) {
       static const Intrinsic::ID StoreInts[] = {Intrinsic::arm_neon_vst1,
                                                 Intrinsic::arm_neon_vst2,
                                                 Intrinsic::arm_neon_vst3,
                                                 Intrinsic::arm_neon_vst4};
 
       static const Intrinsic::ID StoreLaneInts[] = {
         Intrinsic::arm_neon_vst2lane, Intrinsic::arm_neon_vst3lane,
if (Name == "dbg.value" && F->arg_size() == 4) {
       rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::dbg_value);
       return true;
     }
     break;
   }
   case 'e': {
     SmallVector<StringRef, 2> Groups;
-    Regex R("^experimental.vector.reduce.([a-z]+)\\.[fi][0-9]+");
+    static const Regex R("^experimental.vector.reduce.([a-z]+)\\.[fi][0-9]+");
     if (R.match(Name, &Groups)) {
       Intrinsic::ID ID = Intrinsic::not_intrinsic;
       if (Groups[1] == "fadd")
         ID = Intrinsic::experimental_vector_reduce_v2_fadd;
       if (Groups[1] == "fmul")
         ID = Intrinsic::experimental_vector_reduce_v2_fmul;
 
       if (ID != Intrinsic::not_intrinsic) {

llvm/trunk/lib/Passes/PassBuilder.cpp

static cl::opt<bool> EnableUnrollAndJam(
     "enable-npm-unroll-and-jam", cl::init(false), cl::Hidden,
     cl::desc("Enable the Unroll and Jam pass for the new PM (default = off)"));
 
 static cl::opt<bool> EnableSyntheticCounts(
     "enable-npm-synthetic-counts", cl::init(false), cl::Hidden, cl::ZeroOrMore,
     cl::desc("Run synthetic function entry count generation "
              "pass"));
 
-static Regex DefaultAliasRegex(
+static const Regex DefaultAliasRegex(
     "^(default|thinlto-pre-link|thinlto|lto-pre-link|lto)<(O[0123sz])>$");
 
 // This option is used in simplifying testing SampleFDO optimizations for
 // profile loading.
 static cl::opt<bool>
     EnableCHR("enable-chr-npm", cl::init(true), cl::Hidden,
               cl::desc("Enable control height reduction optimization (CHR)"));
 

llvm/trunk/lib/Support/FileCheck.cpp

for (auto PatItr = DagNotStrings.begin(), PatEnd = DagNotStrings.end();
     }
   }
 
   return StartPos;
 }
 
 // A check prefix must contain only alphanumeric, hyphens and underscores.
 static bool ValidateCheckPrefix(StringRef CheckPrefix) {
-  Regex Validator("^[a-zA-Z0-9_-]*$");
+  static const Regex Validator("^[a-zA-Z0-9_-]*$");
   return Validator.match(CheckPrefix);
 }
 
 bool FileCheck::ValidateCheckPrefixes() {
   StringSet<> PrefixSet;
 
   for (StringRef Prefix : Req.CheckPrefixes) {
     // Reject empty prefixes.

llvm/trunk/lib/Support/Regex.cpp

 
 Regex::~Regex() {
   if (preg) {
     llvm_regfree(preg);
     delete preg;
   }
 }
 
-bool Regex::isValid(std::string &Error) const {
-  if (!error)
-    return true;
+namespace {
 
+/// Utility to convert a regex error code into a human-readable string.
+void RegexErrorToString(int error, struct llvm_regex *preg,
+                        std::string &Error) {
   size_t len = llvm_regerror(error, preg, nullptr, 0);
 
   Error.resize(len - 1);
   llvm_regerror(error, preg, &Error[0], len);
+}
+
+} // namespace
+
+bool Regex::isValid(std::string &Error) const {
+  if (!error)
+    return true;
+
+  RegexErrorToString(error, preg, Error);
   return false;
 }
 
 /// getNumMatches - In a valid regex, return the number of parenthesized
 /// matches it contains.
 unsigned Regex::getNumMatches() const {
   return preg->re_nsub;
 }
 
-bool Regex::match(StringRef String, SmallVectorImpl<StringRef> *Matches){
-  if (error)
+bool Regex::match(StringRef String, SmallVectorImpl<StringRef> *Matches,
+                  std::string *Error) const {
+  // Reset error, if given.
+  if (Error && !Error->empty())
+    *Error = "";
+
+  // Check if the regex itself didn't successfully compile.
+  if (Error ? !isValid(*Error) : !isValid())
     return false;
 
   unsigned nmatch = Matches ? preg->re_nsub+1 : 0;
 
   // pmatch needs to have at least one element.
   SmallVector<llvm_regmatch_t, 8> pm;
   pm.resize(nmatch > 0 ? nmatch : 1);
   pm[0].rm_so = 0;
   pm[0].rm_eo = String.size();
 
   int rc = llvm_regexec(preg, String.data(), nmatch, pm.data(), REG_STARTEND);
 
+  // Failure to match is not an error, it's just a normal return value.
+  // Any other error code is considered abnormal, and is logged in the Error.
   if (rc == REG_NOMATCH)
     return false;
   if (rc != 0) {
-    // regexec can fail due to invalid pattern or running out of memory.
-    error = rc;
+    if (Error)
+      RegexErrorToString(error, preg, *Error);
     return false;
   }
 
   // There was a match.
 
   if (Matches) { // match position requested
     Matches->clear();
 
     for (unsigned i = 0; i != nmatch; ++i) {
       if (pm[i].rm_so == -1) {
         // this group didn't match
         Matches->push_back(StringRef());
         continue;
       }
       assert(pm[i].rm_eo >= pm[i].rm_so);
       Matches->push_back(StringRef(String.data()+pm[i].rm_so,
                                    pm[i].rm_eo-pm[i].rm_so));
     }
   }
 
   return true;
 }
 
 std::string Regex::sub(StringRef Repl, StringRef String,
-                       std::string *Error) {
+                       std::string *Error) const {
   SmallVector<StringRef, 8> Matches;
 
-  // Reset error, if given.
-  if (Error && !Error->empty()) *Error = "";
-
   // Return the input if there was no match.
-  if (!match(String, &Matches))
+  if (!match(String, &Matches, Error))
     return String;
 
   // Otherwise splice in the replacement string, starting with the prefix before
   // the match.
   std::string Res(String.begin(), Matches[0].begin());
 
   // Then the replacement string, honoring possible substitutions.
   while (!Repl.empty()) {

llvm/trunk/lib/Target/AArch64/Utils/AArch64BaseInfo.cpp

namespace llvm {
   namespace AArch64SysReg {
 #define GET_SYSREG_IMPL
 #include "AArch64GenSystemOperands.inc"
   }
 }
 
 uint32_t AArch64SysReg::parseGenericRegister(StringRef Name) {
   // Try to parse an S<op0>_<op1>_<Cn>_<Cm>_<op2> register name
-  Regex GenericRegPattern("^S([0-3])_([0-7])_C([0-9]|1[0-5])_C([0-9]|1[0-5])_([0-7])$");
+  static const Regex GenericRegPattern("^S([0-3])_([0-7])_C([0-9]|1[0-5])_C([0-9]|1[0-5])_([0-7])$");
 
   std::string UpperName = Name.upper();
   SmallVector<StringRef, 5> Ops;
   if (!GenericRegPattern.match(UpperName, &Ops))
     return -1;
 
   uint32_t Op0 = 0, Op1 = 0, CRn = 0, CRm = 0, Op2 = 0;
   uint32_t Bits;