> On 7 Jul 2016, at 22:32, Steve Drach <[email protected]> wrote:
>
> Hi,
>
> 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.
>
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?
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.
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?).
588 AtomicBoolean validHolder = new AtomicBoolean();
589 validHolder.set(valid);
Pass valid to the constructor
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.
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?
Change Method to be literally a representation of a single method rather than a
consolidation that is lossy for the set of exceptions.
Paul.
