Thank you for the review Alan. See comments in line below. > Overall I think the API looks much better.
With the advantage of being much simpler too. > 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. I suspected this is a bike shed candidate. I think Release._9 is nicer and it conveys the same information in a less cluttered way than Release.RELEASE_9. > > 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 entries in a legacy jar (the only entries) or in the unversioned section of a multi-release jar are directly under the top-most directory > > 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? Okay > > I assume @since will be added to Release before this is pushed. Yes. > > 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. Both will be fixed. > > Could the legacy JarFile ctor be changed to this(file, verify, mode, > Release.ROOT) to avoid duplication? Yes. > > 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. It’s in the testlibrary under java/util/jar with the other multi-release specific test “helper” classes. I could make it even more specific by putting it under a java/util/jar/multi-release directory > > 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. Do we really have to stick with 80 column hollerith card semantics? Even that was changed to 96 columns about 50 years ago. The one line, other than some “fixmes" that will be removed when JEP 223 is integrated, that exceeds 96 characters long will be changed by wrapping it to 94 columns. >
