I’ve updated the webrev to incorporate the changes suggested by Oleg Barbashov. This changeset has a couple of minor changes to the tests and I’ve changed the Validator class to be an instance of Consumer<JarEntry>.
http://cr.openjdk.java.net/~sdrach/8158295/webrev.03/index.html <http://cr.openjdk.java.net/~sdrach/8158295/webrev.03/index.html> > On Jul 18, 2016, at 6:33 PM, Steve Drach <steve.dr...@oracle.com> wrote: > >>> Please review the following: >>> >>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ >>> <http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/> >>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 >>> <https://bugs.openjdk.java.net/browse/JDK-8158295> >>> >>> This changeset adds a multi-release jar validator to jar tool. After the >>> jar tool builds a multi-release jar, the potential resultant jar file is >>> passed to the validator to assure that the jar file meets the minimal >>> standards of a multi-release jar, in particular that versioned classes have >>> the same api as base classes they override. There are other checks, for >>> example warning if two classes are identical. If the jar file is invalid, >>> it is not kept, so —create will not produce a jar file and —update will >>> keep the input jar file. > > I’ve updated the webrev to address the issues raised — > http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html > <http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html> > >> >> jar/Main.java >> >> 284 // This code would be repeated in the create and update >> sections. >> 285 // In order not to do that, it's here as a consumer. >> 286 Consumer<File> validateAndClose = tmpfile -> { >> Why does it need to be Consumer rather than just a method? > > I changed it to a method. > >> >> Then i think you don’t need to rethow, but you can still keep the try block >> and use finally for File.deleteIfExist since it will not complain for the >> case where you move. > > Done. > >> >> >> 558 int i1 = s1.indexOf('/', n); >> 559 int i2 = s1.indexOf('/', n); >> 560 if (i1 == -1) throw new NumberFormatException(s1); >> 561 if (i2 == -2) throw new NumberFormatException(s2); >> >> I think you are trying to reject entries directly under the versioned area >> so it’s not about numbers (same in the validator, so there is some >> redundancy?). > > A couple things here. The most obvious is it should be “if (i2 == -1)”. I > replaced NumberFormatException with a new InvalidJarException. I added a > comment that > this code is used to sort the version numbers as string representations of > numbers, therefore “9” goes before “10”, not usual for string sorts. > >> >> >> 588 AtomicBoolean validHolder = new AtomicBoolean(); >> 589 validHolder.set(valid); >> >> Pass valid to the constructor > > Done. > >> >> >> Validator/FingerPrint.java >> >> It would be useful to have some comments on what is checked in terms of API >> compatibility. e.g. describe what FingerPrint collects and how Validator >> uses it. > > Added some commentary to FingerPrint. > >> >> >> FingerPrint.java >> >> 205 private final Map<String,Field> fields = new HashMap<>(); >> 206 private final Map<String,Method> methods = new HashMap<>(); >> >> Not sure you need to use a Map, why not use a Set? > > Not sure why I did it with Maps instead of Sets, but I wanted to keep the > method and field names and maybe that made sense at one time. It doesn’t > now, so Sets they are. > >> >> Change Method to be literally a representation of a single method rather >> than a consolidation that is lossy for the set of exceptions. > > Done. > >
