On Feb 13, 2015, at 1:20 AM, Stuart Marks <[email protected]> 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.
