On 10/22/2014 4:58 PM, Claes Redestad wrote:
A minor nit: Package.java line 636: it can return EMPTY_MANIFEST
that will set manifest to non-null so that an invalid file would
avoid opening the file every time getManifest is called (although
this case rarely happens). line 639 can then be simplified.
We need to consider the case where JarInputStream.getManifest()
returns null, which would mean we'd either end up no simpler on line
639 or simply adding a similar check around the value returned from
jis.getManifest(). I also think I saw that referencing the static
EMPTY_PACKAGES from within the privileged anonymous class adds
synthetic access methods...
More importantly, the current approach should already ensure any file
is only scanned once by virtue of always setting manifest to a
non-null value in the end. No?
That's right. I missed that you did set manifest to non-null in line
639. That's fine then.
Yes, I've run the test via JPRT and verified it gets picked up and run
using the default testset, so Windows, Solaris, Mac and Linux should
be covered.
That's good.
This test uses a special class loader that delegates to null class
loader only. Since you have the verification in place, it'd be good
to also call Package.getPackage and Package.getPackages to verify
that the same "package2" instance is returned. As a sanity check,
you could check "java.lang" package must be present.
I've added some sanity checks as suggested. Sadly, it seems that since
the application classloader will define and load package2 first in
this test, there'll always be a Package defined in the
ClassLoader.pkgs that will mask the Package instance in Package.pkgs
when retrieved via the public methods in Package.
Can you remove package2/Class2.class after you create the jar files?
If JDK-8061804 is resolved, we could change to check for identity in
the Package.getPackages() case, which would improve the specificness
of the test... For now, perhaps we could trick things in this test and
put our dummy class into java.lang in our test here and ensure that
the package retrieved is identical. Worth the hassle?
What I meant is to check if the returned Package contains "java.lang"
package that always exists in the system packages. You don't need to
put a dummy class in java.lang to get "java.lang" Package since it must
be defined in the running VM. e.g. you can refactor line 169-176 to a
findPackage method that returns Package matching the given package name
such that in line 164 you can call findPackage("java.lang") that should
return non-null.
Nit: line 35-37, 43-46 - no need to declare these import as
java.lang.* are imported by default.
Some IDEs...
Which IDE are you using? I think it might be your configuration.
Thanks
Mandy
