RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher

[Stuart Marks]

Hi Paul,

I spent some time looking at this API. Overall it seems to me that things work a 
bit more nicely when these methods are added to Pattern instead of Matcher. 
Unfortunately there are some odd things with the existing API that make this 
tradeoff not so obvious.

First, here's what a simple replacement operation looks like when replaceAll() 
is added to Matcher:

         String input = "fooaaaabbfooabbbfoo";
         Pattern p = Pattern.compile("a*b");
         Matcher m = p.matcher(input);
         String result = m.replaceAll(mr -> mr.group().toUpperCase());

But if replaceAll() is on Pattern, we can skip a step:

         String input = "fooaaaabbfooabbbfoo";
         Pattern p = Pattern.compile("a*b");
         String result = p.replaceAll(input, mr -> mr.group().toUpperCase());

Getting stream of match results is similar. So yes, I agree that it simplifies 
things to have these be on Pattern instead of Matcher.

An advantage of having these on Pattern is that the matcher that gets created is 
encapsulated, and its state isn't exposed to being mucked about by the 
application. Thus you can avoid the additional concurrent modification checks 
that you have to do if replaceAll et. al. are on Matcher.

Unfortunately, putting these on Pattern now creates some difficulties meshing 
with the existing API.

One issue is that Matcher already has replaceAll(String) and 
replaceFirst(String). It would be strange to have these here and to have 
replaceAll(replacer) and replaceFirst(replacer) on Pattern.

Another issue is that Matcher supports matching on region (subrange) of its 
input. For example, today you can do this:

     pattern.matcher(input).region(start, end)

The region will constrain the matching for certain operations, such as find() 
(but not replaceAll or replaceFirst). If something like results() were added to 
Matcher, I'd expect that it would respect the Matcher's region, but if results() 
(or matches() as you called it) were on Pattern, the region constraint would be 
lacking.

Also note that Pattern already has this:

     static boolean matches(regex, input)

so I don't think an overload of matches() that returns a Stream would be a good 
idea. (Maybe findAll()?)

Another issue, not directly related to where the new lambda/streams methods get 
added, is that MatchResult allows references only numbered capturing groups. 
Matcher, which implements MatchResult, also supports named capturing groups, 
with the new overloaded methods group(String), start(String), and end(String). 
These were added in Java 7. Logically these also belong on MatchResult, but they 
probably weren't added because of the compatibility issue of adding methods to 
interfaces. Maybe we should add these as default methods to MatchResult.

(But what would the supported implementation be? Just throw 
UnsupportedOperationException?)

--

I'm not entirely sure where this leaves things. It certainly seems more 
convenient to have the new methods on Pattern. But given the way the existing 
API is set up, it seems like it's a better fit to add them to Matcher.

s'marks



On 2/9/15 6:18 AM, Paul Sandoz wrote:
> Here is an alternative that pushes the methods on to Pattern instead:
>
>    http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/on-Pattern/webrev/
>
> (Whe webrev reports some files as empty, please ingore those, i have this webrev stacked on the previous one.)
>
> I have also included replaceFirst.
>
> This simplifies things for streaming on the match results and also for replacing.
>
> Note that the existing replace* methods on Matcher reset the matcher before matching and indicate that the matcher should be reset afterwards for reuse. Thus there is no loss in functionality moving such lambda accepting methods from Matcher to Pattern. It comes down to the performance of reusing a matcher, which does not seems so compelling to me.
>
> Paul.
>
> On Feb 5, 2015, at 11:59 AM, Paul Sandoz <Paul.Sandoz at oracle.com> wrote:
>
>> Hi.
>>
>> Please review these stream/lambda enhancements on Matcher:
>>
>>   http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/webrev/
>>
>> Two new methods are added to Matcher:
>>
>> 1) replaceAll(Function<MatchResult, String> ) that is more flexible than the existing replaceAll that accepts a single value.
>>
>> 2) Stream<MatchResult> results() that returns a stream of MatchResult for all matches.
>>
>> The former does introduce a minor source incompatibility for a null argument, but then so did the new append methods accepting StringBuilder that were recently added (see JDK-8039124).
>>
>> For the latter i opted to place the method on Matcher rather than Pattern as i think that is a better fit with current usages of Matcher and operating on a MatchResult. That marginally increases the complexity since co-modification checking is required.
>>
>> I update the test PatternStreamTest to derive the expected result.
>>
>>
>> I suppose i could add another method replaceFirst(Function<MatchResult, String> ) if anyone feels strongly about that. Consistency-wise it seems the right thing to do.
>>
>> Paul.
>