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

[David Blaikie via llvm-commits]

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.

On Thu, Sep 1, 2016 at 3:47 PM Rui Ueyama <ruiu at google.com> wrote:

> 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/20160902/6a331020/attachment.html>