Re: Code review for jigsaw/jake -> jdk9/dev sync up

test/java/security/testlibrary/Proc.java: if (hasModules) { Stream.of(jdk.internal.misc.VM.getRuntimeArguments()) -.filter(arg -> arg.startsWith("--add-exports=")) +.filter(arg -> arg.startsWith("--add-exports=") || +

Re: Code review for jigsaw/jake -> jdk9/dev sync up

On 28/11/2016 14:47, Chris Hegarty wrote: : 2) jartool Main.java Maybe concealedPackages should have a comment about its use ( it is used in the Validator, and not by Main at all ). Just on this one, I think this was introduced when Steve added the MR JAR validation, I agree it's ugly

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Thanks Lois. I removed the blank line. Mandy > On Nov 28, 2016, at 6:32 AM, Lois Foltan wrote: > > Hi Alan, > > I have reviewed the hotspot changes and they look good. Minor nit, > src/share/vm/classfile/javaClasses.cpp only differs by the addition of a > blank

RE: Code review for jigsaw/jake -> jdk9/dev sync up

Hi Alan, I've reviewed the test changes for Hotspot and they look good. Thanks, Christian -Original Message- From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-boun...@openjdk.java.net] On Behalf Of Alan Bateman Sent: Thursday, November 24, 2016 10:25 AM To: jigsaw-dev

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Karen, Sorry for delay. I was on vacation last week. I plan to review the changes tomorrow. -Dmitry On 2016-11-28 17:47, Karen Kinnear wrote: > Alan, > > I reviewed all the hotspot runtime changes > - except the tests (Christian will review those) > - and jvmti - which Dmitry Samersoff will

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Hi, I’ve reviewed Langtools code. There are various comment “//TODO”, “//FIXME”, “//XXX”. I think they should be revised. May be issues should be filed to track them. Unused import at 37 import java.io.IOException; in langtools/test/tools/javac/modules/ModuleInfoTest.java ASCII graphics issue

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Thanks for going through the changes, a few comment/replies below. On 28/11/2016 22:22, Paul Sandoz wrote: : What happens if you pass a primitive array? I think you need to specify what happens if an array class is passed and how the target class is obtained, and an IAE if the "element

Re: Code review for jigsaw/jake -> jdk9/dev sync up

For the JDK patch: MethodHandles — 176 public static Lookup privateLookupIn(Class targetClass, Lookup lookup) throws IllegalAccessException { 177 SecurityManager sm = System.getSecurityManager(); 178 if (sm != null) sm.checkPermission(ACCESS_PERMISSION); 179 if

Re: Code review for jigsaw/jake -> jdk9/dev sync up

On 11/28/16 3:28 AM, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? I agree that "opens" leveraging a type named "Exports..." is

Re: Code review for jigsaw/jake -> jdk9/dev sync up

On 28/11/16 14:53, Jan Lahoda wrote: Thanks for the comments Maurizio. On 28.11.2016 12:28, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Thanks for the comments Maurizio. On 28.11.2016 12:28, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? It seemed to me that having

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Reviewed Nashorn changes. All fine. -Sundar On 11/28/2016 8:17 PM, Chris Hegarty wrote: > On 24 Nov 2016, at 15:25, Alan Bateman wrote: >> ... >> To get going, I've put the webrevs with a snapshot of the changes in jake >> here: >>

Re: Code review for jigsaw/jake -> jdk9/dev sync up

On 24 Nov 2016, at 15:25, Alan Bateman wrote: > > ... > To get going, I've put the webrevs with a snapshot of the changes in jake > here: >http://cr.openjdk.java.net/~alanb/8169069/0/ Overall this look very good. I ran through most of the changes in the jdk repo,

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Hi Alan, I have reviewed the hotspot changes and they look good. Minor nit, src/share/vm/classfile/javaClasses.cpp only differs by the addition of a blank line. Thanks, Lois On 11/24/2016 10:25 AM, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? * ClassReader: should we have checks regarding an open module containing no open directives in the classfile?

Re: Code review for jigsaw/jake -> jdk9/dev sync up

On 27/11/2016 11:15, Peter Levart wrote: Hi Alan, Overall this looks very good. I noticed a couple of nits... Thanks for going through the changes. : So should Provider rather declare the following? Class type(); Or alternatively, should ServiceLoader rather declare the following?

Re: Code review for jigsaw/jake -> jdk9/dev sync up

Hi Alan, Overall this looks very good. I noticed a couple of nits... 1. I wonder if the new ServiceLoader API signature should be tweaked a bit... There is a new method in ServiceLoader with the following signature: public Stream stream() ...where Provider declares the