On 10/12/2014 04:09 AM, Claes Redestad wrote:


http://cr.openjdk.java.net/~redestad/8060130/webrev.03/


Hi Claes,

First, one more nit:

1635                 if (!map.containsKey(pkgName)) {
1636                     map.put(pkgName, pkg);
1637                 }

..could be written as:

map.putIfAbsent(pkgName, pkg);



And second, I re-read the code of CachedManifest and noticed the following. You initialize the volatile instance field 'manifest' like that:

 609         private volatile Manifest manifest = EMPTY_MANIFEST;


So in following code:

 631         public Manifest getManifest() {
 632             resolveManifest();
 633             return manifest;
 634         }
 635
 636         private void resolveManifest() {
 637             if (manifest != null || url == null) {
 638                 return;
 639             }
 640             synchronized(this) {
 641                 if (manifest == null) {
 642                     manifest = AccessController.doPrivileged(new 
PrivilegedAction<Manifest>() {
 643                         public Manifest run() {
 644                             return loadManifest(fileName);
 645                         }
 646                     });
 647                 }
 648             }
 649         }



... loadManifest() will never be executed. getManifest() will always return EMPTY_MANIFEST.

You probably wanted line 637 to be written as:

if (manivest != EMPTY_MANIFEST || url == null) { ...


Likewise in loadManifest(), wou write:

 586     private static Manifest loadManifest(String fn) {
 587         try (FileInputStream fis = new FileInputStream(fn);
 588              JarInputStream jis = new JarInputStream(fis, false))
 589         {
 590             return jis.getManifest();
 591         } catch (IOException e) {
 592             return EMPTY_MANIFEST;
 593         }
 594     }

..but you probably wanted the line 592 to return 'null' instead.


Regards, Peter

Reply via email to