Thank you Alan. I’ll address the issues you bring up before integration.
> On Feb 15, 2016, at 4:30 AM, Alan Bateman <alan.bate...@oracle.com> wrote: > > > On 10/02/2016 01:04, Steve Drach wrote: >> Hi, >> >> Yet another webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ >> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>, with a change to >> JarEntryIterator to fix a problem discovered by performance tests — calling >> hasNext() twice as often as needed. I also removed the @since 9 tags from >> the methods entries() and stream(), and added an additional sentence to the >> spec for each of those methods that clarifies what a base entry is (actually >> is not). >> > I went through the latest webrev and it looks quite good. > > A few comments on the javadoc: > > "... partitioned by the major version of Java platform releases" - this might > be better as "... partitioned by the major version of the Java platform". > > In JarFile.Release then it uses the phrase "top-most (base) directory". I > thought we had purged "top-*" from the javadoc in previous iterations because > it hints of classes or resources in the top most directory (which isn't the > case with classes in a named package). > > "... will not be accessible by this JarFile" hints of access control or even > security manager. Would it clearer to re-word this to something like "will > not be located by methods such as getEntry" ? > > "returned depends whether" -> "returned depends on whether". > > In the javadoc for entries() and stream() then it mentions "the constructor" > many times. I would be tempted to replace many of these - for example "all > entries are returned, regardless of the constructor" might be better as "all > entries of returned, regards of how the JarFile is created". > > A couple of nits on the implementation: > > vze = JarFile.super.getEntry(META_INF_VERSIONS + i-- + sname); > - it would be more readable if you move the decrement to its own line. Also I > assume that JarFile is not needed here. > > L942-943 looks messy too, I assume that can be cleaned up. > > JarFileFactory - "earl" will confuse readers, needs a comment or a better > name. > > I think this is all that I have for now. > > -Alan.
