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

[David Blaikie via llvm-commits]

On Thu, Sep 1, 2016 at 12:59 AM George Rimar <grimar at accesssoftek.com>
wrote:

> 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".
>

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.


>
> > 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.
>

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.

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.


>
> 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.
>
>
> https://reviews.llvm.org/D24102
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160901/f7dd00fd/attachment.html>