Hi
It might be more consistent with the existing design to add those
methods into Matcher. I agree the
better name for the "stream return" method is "findAll".
-sherman
On 2/11/15 2:25 AM, Paul Sandoz wrote:
Hi Stuart,
Thanks for the detailed review.
Here is a possible way forward:
1) Add the methods to Matcher, as proposed in the initial webrev.
1.1) Change the specification of Matcher.results to reset the stream before
matching, making it consistent with the replace* methods.
2) Add convenience methods for all replace*() and matches() on Pattern that
defer to those on Matcher.
We can do that in two stages, focusing on 1) in this review.
I was not too concerned about the static method and the stream returning method having the same
name as the context is quite different. For stream returning methods there is a de-facto pattern of
using a plural of the stream element kind, so i prefer that to findAll. What about the name
"Pattern.matchResults"? which chains well with "Pattern.match(...).results()".
--
Regarding the disparity between MatchResult and Matcher. I think that would
require a new sub-interface of MatchResult from which Matcher extends from and
returns. If we think it important we should do that for 9 otherwise we will be
stuck for the stream-based methods.
Paul.
On Feb 11, 2015, at 2:02 AM, Stuart Marks <stuart.ma...@oracle.com> wrote:
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.san...@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.
