Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-27 Thread Paul Sandoz
On Aug 25, 2012, at 12:57 AM, Mandy Chung mandy.ch...@oracle.com wrote: On 8/24/2012 3:44 PM, David Holmes wrote: My other query with these changes is whether we are certain that using the specified loader rather than the boot loader will always be correct. Yes I'm to my best knowledge

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-24 Thread Paul Sandoz
Hi Mandy, --- old/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug 23 12:29:01 2012 +++ new/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug 23 12:29:00 2012 @@ -52,7 +52,7 @@ AccessController.doPrivileged(new PrivilegedActionIIOPProxy() {

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-24 Thread Rémi Forax
I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi On 08/24/2012 07:21 AM, serguei.spit...@oracle.com wrote: I was thinking about the same. But a CCC request is needed for such a

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-24 Thread Alan Bateman
On 24/08/2012 11:25, Rémi Forax wrote: I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi I think this is worth looking at too. It would only change two or three usages in this

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-24 Thread David Holmes
On 25/08/2012 1:53 AM, Mandy Chung wrote: On 8/23/2012 6:27 PM, David Holmes wrote: Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-24 Thread Mandy Chung
On 8/24/2012 3:44 PM, David Holmes wrote: My other query with these changes is whether we are certain that using the specified loader rather than the boot loader will always be correct. Yes I'm to my best knowledge but I'm looking to the reviewers to tell me otherwise :) The class being

Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Mandy Chung
This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at:

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Alan Bateman
On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Mandy Chung
On 8/23/2012 11:58 AM, Alan Bateman wrote: On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread serguei.spit...@oracle.com
Mandy, It looks good. Just a question below. || *src/share/classes/java/lang/management/ManagementFactory.java* 596 String intfName = mxbeanInterface.getName(); 599 is not an instance of + mxbeanInterface); Did you want this?: 596 String intfName

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Mandy Chung
On 8/23/2012 12:48 PM, serguei.spit...@oracle.com wrote: Mandy, It looks good. Thanks. Just a question below. || *src/share/classes/java/lang/management/ManagementFactory.java* 596 String intfName = mxbeanInterface.getName(); 599 is not an instance of +

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Alan Bateman
On 23/08/2012 20:33, Mandy Chung wrote: : Done. This is a good cleanup I considered to do too but missed in the previous webrev. http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/ Thanks for the review. Mandy That looks much better, so looks good to me. -Alan

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread Dmitry Samersoff
Mandy, 1. You replace null with getClassLoader() calls in couple of places. getClassLoader requires special permissions - RuntimePermission(getClassLoader) so probably you need to use doPrivileget() there. Did you test your changes with SecurityManager/No permissions for the test

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread serguei.spit...@oracle.com
Looks good. Thanks, Serguei On 8/23/12 12:33 PM, Mandy Chung wrote: On 8/23/2012 11:58 AM, Alan Bateman wrote: On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread David Holmes
Hi Mandy, While I understand what you are doing and that it works it is far from obvious upon code inspection that where you replace null with Foo.class.getClassLoader(), that the result would always be null. Another way to simplify this would be to add a new overload of Class.forName():

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

2012-08-23 Thread serguei.spit...@oracle.com
I was thinking about the same. But a CCC request is needed for such a change in public API. It can be done separately if any other API changes are necessary. Thanks, Serguei On 8/23/12 6:27 PM, David Holmes wrote: Hi Mandy, While I understand what you are doing and that it works it is far