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

Reply via email to