>> There is a new webrev at
>> http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/
>
> sun/tools/jar/Main.java
>
> Thanks for refactoring and adding the findConcealedPackages method. What I
> actually meant was to move out this line:
> concealedPackages = findConcealedPackages(rd);
>
> to probably before calling addExtendedModuleAttributes(moduleInfos) above
> line 342 and 1101.
I tried that and decided it was non-optimal because we’d have to construct the
ModuleDescriptor from the modInfos twice in succession. Let's compromise here,
okay?
>
> 2014 .filter(p -> !p.equals("”))
>
> For a modular JAR, there should be no unnamed package. I think the jar tool
> should fail if it detects an unnamed package. Your test does not have any
> unnamed package - how did you find this?
the modularJar/Basic test found a bug. Then when I was fixing the bug in
toPackageNames I noticed it could return unnamed packages (“”). And in fact
there are some, a few, classes that aren’t in a package. Jar tool shouldn’t
fail with unnamed packages. They could even exist in a modular multi-release
jar when the module-info class is only in a versioned directory. I guess it
should fail if a class in an unnamed package is in a module, although I’m not
even sure about that.
>
> ConcealedPackage.java test
>
> Thanks for improving the test. It’d be good to name the @Test method with a
> descriptive method name e.g.
> test1 -> testUpdateVersionedPublicClass
> test2 -> testUpdatedVersionedPublicConcealedClass
It’s difficult to come up with names that aren’t sentences. I figured the
comments would explain the test adequately.
>
> 117 @Test // updates a valid multi-release jar with a new public class in
> 118 // versioned section and fails
>
> Nit: You can consider moving the comment above @Test.
Ok
