On 20/01/2016 17:56, Steve Drach wrote:
Hi,
This is a repeat of the RFR I sent last Wed (Jan 13).
:
Webrev: http://cr.openjdk.java.net/~sdrach/8132734/webrev.02/
<http://cr.openjdk.java.net/~sdrach/8132734/webrev.02/>
Overall I think the API looks much better.
For Release then I have to admit that I dislike _9 and wonder if other
options were considered? javax.lang.model.SourceVersion uses the
RELEASE_xx convention for example.
Also I wonder about Release.ROOT and whether Release.UNVERSIONED was
considered? In general the phrase "root entry" in the javadoc makes me
think the root or top-most directory. An alternative that might be
clearer is to say "unversioned entry" and define that term clearly in
the class description.
The javadoc for Release.RUNTIME looks like it will have a javadoc link
to jdk.Version but that is a JDK-specific API. Could the text starting
"The effective runtime ..." move to an @implNote?
I assume @since will be added to Release before this is pushed.
The new JarFile ctor doesn't seem to handle the case that version is
null when multi release is forced. Also I assume it should specify
@throws SecurityException.
Could the legacy JarFile ctor be changed to this(file, verify, mode,
Release.ROOT) to avoid duplication?
I don't have time to do a detailed pass over the updated tests but I
wonder if SimpleHttpServer is really a candidate to put in the
testlibrary tree. It looks like it is very specific to multi-release
JARs and so I would expect to be co-located with those tests rather than
being a hazard in the testlibrary tree.
A small comment is that it would be good to fix the really long lines
before these changes are pushed. This will help future side-by-side
reviews where it would be otherwise annoying to have to do horizontal
scrolling to view diffs.
-Alan.
