RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package

[Mandy Chung]

Claes, Peter,

   Thanks for the revised webrev and Peter's thorough review.  webrev.05
looks much better.  My comment is mostly minor.

On 10/13/2014 8:41 AM, Claes Redestad wrote:
> http://cr.openjdk.java.net/~redestad/8060130/webrev.05

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(...)

line 621-623: is this really needed?  Uncontended case seems to be
    the common case.  It seems the synchronized overhead would be
    insignificant.

line 624: a space is missing between synchronized and "("

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?

Thanks
Mandy