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

[Rui Ueyama via llvm-commits]

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?

On Thu, Sep 1, 2016 at 3:25 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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/9595a94e/attachment.html>