RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Claes Redestad
claes.redestad at oracle.com
Tue Oct 21 13:43:07 UTC 2014
Hi Mandy,
thanks for the review!
On 10/15/2014 03:07 AM, Mandy Chung wrote:
> Claes, Peter,
>
> Thanks for the revised webrev and Peter's thorough review. webrev.05
> looks much better. My comment is mostly minor.
>
>
> ClassLoader.java
>
> line 1582-1586 - I suggest to get rid of the "oldpkg" variable
> (it's really the package to be used and not an old pkg).
>
> pkg = new Package(name, specTitle, specVersion, specVendor,
> implTitle, implVersion, implVendor,
> sealBase, this);
> if (packages.putIfAbsent(name, pkg) != null) {
> throw new IllegalArgumentException(name + " already defined");
> }
> return pkg;
>
> line 1634-1635: nit: the pkgName variable is not really needed.
> it's in the existing code and probably good to remove it.
> Package.java
>
> line 473: maybe better to leave the ClassLoader parameter in the
> constructor.
> I thought about adding a comment saying that this private constructor
> is only used for system package but keeping the loader parameter makes
> it more explicit.
>
> line 569: nit: formatting - indent to the right to align the first
> parameter
> to new Package(...)
Fixed.
>
> line 621-623: is this really needed? Uncontended case seems to be
> the common case. It seems the synchronized overhead would be
> insignificant.
I'd prefer sticking to the double-checked idiom here, unless you insist.
I've cleaned up the code to avoid assignment in if-clauses, which
according to
anonymous sources makes the code more readable. Perhaps this addresses
some of your concerns?
>
> line 624: a space is missing between synchronized and "("
Fixed.
>
> Looks like there is one test
> jdk/test/java/lang/ClassLoader/GetPackage.java
> about packages. Can you add a new test to verify the system packages
> as that
> is one major change in your patch?
http://cr.openjdk.java.net/~redestad/8060130/webrev.06
Since I don't want to add binary JAR files, I opted to add a test which
creates two JAR files,
each with a single class (with/without manifest) and then proceeds to
spawn processes
to verify that:
- when the JAR with manifest is on bootclasspath, the package can be
found via
Package.getSystemPackage and the package object reflects values added to
the manifest
- when the JAR without manifest is on bootclaspath, the package can be
found via
Package.getSystemPackage but is empty apart from name
- adding the test.classes directory to bootclasspath behaves the same as
adding the JAR
without manifest
- for any case where the class/package is not on the bootclasspath, the
package information
can not be found via Package.getSystemPackage().
Does this cover everything?
I guess there might be a way to make @run main/othervm or
main/bootclasspath pick
up a dynamically generated JAR file, but I've failed to find a way that
would make this work without
pregenerating the JARs. Suggestions on how this can be simplified are
welcome.
/Claes
>
> Thanks
> Mandy
>
More information about the core-libs-dev
mailing list