[7u4] Review request for CR 7134730 (original) (raw)

Kumar Srinivasan kumar.x.srinivasan at oracle.COM
Mon Feb 13 10:02:07 PST 2012

Hi Greg,

I scanned the webrev, here are my comments:

  1. General comments the copyright is not formatted right, it is now: 2 * Copyright 2012 Oracle and/or its affiliates. All rights reserved.

it should be, note the comma after the year.

2 * Copyright 2012, Oracle and/or its affiliates. All rights reserved.

and several files missing Copyrights.

  1. main.m // disclaimer I don't fully understand this language (Objective C ?) FULL_VERSION and DOT_VERSION is always is hard-coded to 1.7.0 what happens when we go to 1.8.0 ? Can we not set this in some automated way.

  2. It looks the cp_wildcard expansion is set to TRUE in JLI_LaunchfFxnPtr, do we really need this ? the classpath is going to be hard-coded right ? IMO we don't require cp wildcarding, we can set that to FALSE.

  3. I also find the version string hard-coded in build.xml, we need to have a mechanism such that version information is derived from the make/build logic.

  4. The test is great, but how will the test be executed ? typically in the jdk, the tests are located under jdk/test/......, and these are all run by jtreg automatically on a periodic basis (nightly, weekly, promotion etc), we need this hooked up into that framework.

  5. AppBundlerTask.java.html

Does the ant code get built and run with jdk6 ? it that is the case then the following may be moot, but of course you can use for-each and simplify some code below.

71 private ArrayList classPath = new ArrayList(); 72 private ArrayList nativeLibraries = new ArrayList(); 73 private ArrayList options = new ArrayList(); 74 private ArrayList arguments = new ArrayList();

you can set the assiged type to List. private List classPath = new ArrayList<>();

lines 135....

135 public void addConfiguredClassPath(FileSet classPath) { 136 File parent = classPath.getDir(); 137 138 DirectoryScanner directoryScanner = classPath.getDirectoryScanner(getProject()); -139 String[] includedFiles = directoryScanner.getIncludedFiles(); 140 -141 for (int i = 0; i< includedFiles.length; i++) { -142 this.classPath.add(new File(parent, includedFiles[i])); -143 } for (String name : directoryScanner.getIncludedFiles() { this.classPath.add(new File(parent, name); } 144 }

similarly lines 453 and 477.

Test.java, I would make the test program dump out the args as well, and pass some dummy args to ensure that args are passed correctly to the app's main.

I noticed that you set the starting heapsize and maximum heap size ?

Kumar

Why are these three files in the webrev not under the standard GPL license used for the rest of OpenJDK (and the rest of the files in the webrev)?:

http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/build.xml.html http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/appbundler/native/main.m.html http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/build.properties.html An oversight. They should be - I will add the GPL header. G

More information about the macosx-port-dev mailing list