[LLVM/Support] - Disallow match() method of llvm::Regex to change object state.

[grimar]

Summary

match() method previously was able to change the error filed of a object.
That looked as excessive because pattern is compiled in constructor,
so to the moment of match() call should be already known if Regex state is valid or not.

match() returns a bool flag what should be enough to know was there some fail during the call or not.

This change allows to make match() to be const what is useful if we have constant reference to Regex
and want to call match().

[dblaikie]

I'm not as comfortable with this change - have you looked around to see if any callers are actually checking the error after calling Match?

It seems like a client could have a problem in either case (if Regex says "no match" when it's really "ran out of memory" then maybe the code would behave incorrectly - assuming a match wasn't present when it may actually have been present).

[grimar]

In D24102#530947, @dblaikie wrote:

I'm not as comfortable with this change - have you looked around to see if any callers are actually checking the error after calling Match?

Yes, sure. Callers in llvm check only returned bool value from what I saw. Most common scenarios btw is to create temporarily Regex in place and call match() like:

if (Regex(...).match()) {...

So they check only returned bool and do not care why it is false, which is fine in my opinion, as I don't expect match() to return false because of something except the "no match found".

It seems like a client could have a problem in either case (if Regex says "no match" when it's really "ran out of memory" then maybe the code would behave incorrectly - assuming a match wasn't present when it may actually have been present).

Even if we would like to handle OOM error (though we dont do that in llvm generally), there is no good way to do that atm. The same can be applied for any specific error, because
the only API Regex provides for diagnostics is bool isValid(std::string &Error), so callers can only compare returned string with "out of memory", what does not look as usable API.

So what I want to say is that I believe it is not supposed to use error member currently for anything else then text error reporting.
And callers do that earlier than match() call (if they do):

Regex RegEx(ExtractRegExpGlobals[i]);
if (!RegEx.isValid(Error)) {
  errs() << argv[0] << ": '" << ExtractRegExpGlobals[i] << "' "
    "invalid regex: " << Error;
}

So since at the moment of match() call it should be already known if regex was valid or not, and nobody seems to use IsValid() after match(), I think it is correct and useful cleanup change.

[grimar]

include/llvm/Support/Regex.h

78–79

btw, code in Regex.h and .cpp is not clang-formatted.
In this patch, formatting can affect on this line, should I format it ?

[grimar]

• clang-formatted.
• fixed last minute mistype in condition.

[grimar]

• Simplified.

[grimar]

• Fixed the same mistype again. Sorry :/

[grimar]

Change was intended to use in lld, but it is no more needed atm.

include/llvm/Support/Regex.h

public:
 
     /// 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.
     ///
     /// This returns true on a successful match.
-    bool match(StringRef String, SmallVectorImpl<StringRef> *Matches = nullptr);
+    bool match(StringRef String,
+               SmallVectorImpl<StringRef> *Matches = 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).
     ///

lib/Support/Regex.cpp

 }
 
 /// 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){
+bool Regex::match(StringRef String, SmallVectorImpl<StringRef> *Matches) const {
   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);
-
-  if (rc == REG_NOMATCH)
-    return false;
-  if (rc != 0) {
-    // regexec can fail due to invalid pattern or running out of memory.
-    error = rc;
+  if (rc != 0)
     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) {