<div dir="ltr">If it doesn't have a non-exception based mode/option/usage, we might not be able to migrate to it - I'm not sure, really.</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Sep 1, 2016 at 3:47 PM Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">std::regex doesn't have something that corresponds to the error stat, and instead it seems to use exceptions to notify errors. When we migrate to std::regex, how would we handle errors?</div></div><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 1, 2016 at 3:25 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Thu, Sep 1, 2016 at 12:59 AM George Rimar <<a href="mailto:grimar@accesssoftek.com" target="_blank">grimar@accesssoftek.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D24102#530947" rel="noreferrer" target="_blank">https://reviews.llvm.org/D24102#530947</a>, @dblaikie wrote:<br>
<br>
> 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?<br>
<br>
<br>
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:<br>
<br>
  if (Regex(...).match()) {...<br>
<br>
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".<br></blockquote><div><br></div></span><div>I'm not sure I follow - my point you said you "don't expect match() to return false becouse of something except the 'no match found'" - but this API (currently, and moreso with your change) /does/ return false for other reasons, outside your (& other people's) Expectations. That seems problematic.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 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).<br>
<br>
<br>
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<br>
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.<br></blockquote><div><br></div></span><div>I'm not suggesting the code would have to know that it was an OOM state, just a (vague) error state. If the caller assumed that "false" meant "no match" when it really meant "some problem occurred and it's unknown whether there is or is not a match", then the program could behave incorrectly.<br><br>This API seems a bit perilous, and I'm not sure we should be hiding more error states, rather than going the other direction and making them more visible.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br>
And callers do that earlier than match() call (if they do):<br>
<br>
  Regex RegEx(ExtractRegExpGlobals[i]);<br>
  if (!RegEx.isValid(Error)) {<br>
    errs() << argv[0] << ": '" << ExtractRegExpGlobals[i] << "' "<br>
      "invalid regex: " << Error;<br>
  }<br>
<br>
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.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D24102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D24102</a><br>
<br>
<br>
<br>
</blockquote></span></div></div>
</blockquote></div><br></div></div></blockquote></div>