RFR: Proposed jimage refresh for JDK9

[Jim Laskey (Oracle)]

> On May 19, 2015, at 8:03 AM, Remi Forax <forax at univ-mlv.fr> wrote:
> 
> As Paul said,
> it's a big patch …

Agree.

> 
> In ImageBuilder, Option should take the description as 3rd parameter instead of overriding description.
> Once this is done, Option should not be abstract but takes the processing as an argument too
> (as a lambda). i.e instead of
> 
> new Option(true, "--cmds") {
>            @Override
>             void process(ImageBuilder task, String opt, String arg) throws BadArgs {
>                 task.options.cmds = splitPath(arg, File.pathSeparator);
>             }
>            @Override
>             String description() { return "Location of native commands"; }
>         }
> }
> 
> it's better to write:
> new Option(true, "--cmds", "Location of native commands",
>    (ImageBuilder task, String opt, String arg) -> {
>      task.options.cmds = splitPath(arg, File.pathSeparator);
>    })
> 
> the code is ligther, it uses delegation instead of inheritance and remove a dozen of inner classes on disk.

Done.

> 
> In ModuleArchive, close() should continue to close the opened input streams even if one throw an exception,
> @Override
> public void close() throws IOException {
>      IOException e = null;
>      for(InputStream stream : opened) {
>           try {
>             stream.close();
>          }catch(IOException ex) {
>             if (e == null) {
>               e = ex;
>             } else {
>               e.addSuppressed(ex);
>             }
>          }
>      }
>      if (e != null) {
>        throw e;
>      }
> }

Done.

> 
> In BasicImageWriter, getString can be simplified to:
> public String getString(int offset) {
>    UTF8String utf8 = strings.get(offset);
>    if(utf8 != null) {
>         return utf8.toString();
>    }
>    return null;
> }

Done.

> 
> also in generatePerfectHash, you don't need to create a Stream to do a forEach on it:
>  input.stream().forEach(
> you can write
>  input.forEach(

Done.

> 
> ImageReader.LinkNode, instead of @SuppressWarnings("LeakingThisInConstructor"),
> the constructor should be private and you should add a static factory method that
> does parent.addChild(linkNode) after calling the constructor.
> 

Done.

> in JrtFileAttributeView.get() that takes a Class, you forget to create the JrtFileAttributeViews
> with options as 3 parameters (as you do in get that takes a String).
> 
> in JrtPath, instead of
>  boolean isSame = Arrays.equals(this.getResolvedPath(),
>                              ((JrtPath)other).getResolvedPath());
>  return isSame? isSame : jrtfs.isSameFile(this, (JrtPath)other);
> the usual code is:
>  JrtPath path = (JrtPath) other;
>  return Arrays.equals(this.getResolvedPath(), path.getResolvedPath()) ||
>             jrtfs.isSameFile(this, path);
> 

Done

> and I fully agree with the review of Paul :)
> 
> Rémi
> 
> On 05/15/2015 05:45 PM, Jim Laskey (Oracle) wrote:
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/top/ <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/top/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/hotspot/ <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/hotspot/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/jdk/ <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/jdk/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/langtools/ <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/langtools/>
>> 
>> There are some issues with regard to the hotspot changes, but I’m working with Coleen and John R. to resolve those.
>> 
>> Cheers,
>> 
>> — Jim
>> 
>