OK, this looks great. Thanks for the updates.
There is also
"in same order" -> "in the same order"
in the doc for the results() method, as Brian pointed out internally.
No need for another webrev.
s'marks
On 2/13/15 1:17 AM, Paul Sandoz wrote:
On Feb 13, 2015, at 1:20 AM, Stuart Marks <stuart.ma...@oracle.com> wrote:
On 2/12/15 3:15 AM, Paul Sandoz wrote:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/webrev/
OK, overall looks pretty good. Two minor comments on Matcher.java:
1202 if (expectedCount >= 0 && expectedCount != matchOrResetCount)
1203 return true;
This is a concurrent modification check, so other cases that test this
condition will throw CME. Should this also throw CME or should it indeed return
true?
The latter, so a CME is only thrown on data returning methods.
I have added a comment:
// Defer throwing ConcurrentModificationException to when
// next is called. The is consistent with other fail-fast
// implementations.
if (expectedCount >= 0 && expectedCount != matchOrResetCount)
return true;
Given that the iterator is never exposed directly it most likely does not
matter, but i wanted to be consistent.
1224 // Perform a first find if required
1225 if (s < 1 && !find())
1226 return;
If I understand this correctly, the state field can have values -1, 0, or 1;
and 's' is a local variable that's initialized from the state field.
I was confused by this code because it ends up calling find() when s == 0, which means
"not found" ... so why is find() being called again in this case?
However, the s == 0 case is dispensed with earlier, so it actually cannot occur
here. I think it would be clearer, and have the same behavior, if the condition
were changed to
if (s < 0 && !find())
Ok.
Webrev updated in place:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/webrev/src/java.base/share/classes/java/util/regex/Matcher.java.sdiff.html
Thanks,
Paul.
