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

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 1 00:58:56 PDT 2016

grimar added a comment.

In https://reviews.llvm.org/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.


More information about the llvm-commits mailing list