> On Jul 28, 2016, at 11:59 AM, Oleg G. Barbashov <[email protected]> > wrote: > > 27.07.2016 15:34, Oleg G. Barbashov пишет: >> >> 07.07.2016 23:32, Steve Drach пишет: >>> 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. >>> >>> Thanks >>> Steve >> >> 574 private boolean validate(String fname) { >> 575 boolean valid = true; >> 576 Validator validator = new Validator(this); >> 577 >> 578 try (JarFile jf = new JarFile(fname)) { >> 579 AtomicBoolean validHolder = new AtomicBoolean(valid); >> 580 jf.stream() >> 581 .filter(e -> !e.isDirectory()) >> 582 .filter(e -> !e.getName().equals(MANIFEST_NAME)) >> 583 .filter(e -> !e.getName().endsWith(MODULE_INFO)) >> 584 .sorted(entryComparator) >> 585 .forEachOrdered(je -> { >> 586 boolean b = validator.accept(je, jf); >> 587 if (validHolder.get()) validHolder.set(b); >> 588 }); >> 589 valid = validHolder.get(); >> 590 } catch (IOException e) { >> 591 error(formatMsg2("error.validator.jarfile.exception", fname, >> e.getMessage())); >> 592 valid = false; >> 593 } catch (InvalidJarException e) { >> 594 error(formatMsg("error.validator.bad.entry.name", >> e.getMessage())); >> 595 valid = false; >> 596 } >> 597 return valid; >> 598 } >> >> (IMHO) Using of AtomicBoolean and forEachOrdered() for stateful validator >> here looks forced. It may be avoided by using regular iterator with "for" >> loop: >> >> Stream<JarEntry> sorted = jf.stream() >> .filter(e -> !e.isDirectory()) >> .filter(e -> !e.getName().equals(MANIFEST_NAME)) >> .filter(e -> !e.getName().endsWith(MODULE_INFO)) >> .sorted(entryComparator); >> for (Iterator<JarEntry> iter = sorted.iterator(); >> iter.hasNext();) { >> if (!validator.accept(iter.next(), jf)) { >> valid = false; >> } >> } >> >> or even better by storing "valid" state inside the Validator after turning >> it to regular Consumer<JarEntry>: >> >> try (JarFile jf = new JarFile(fname)) { >> Validator validator = new Validator(this, jf); >> jf.stream() >> .filter(e -> !e.isDirectory()) >> .filter(e -> !e.getName().equals(MANIFEST_NAME)) >> .filter(e -> !e.getName().endsWith(MODULE_INFO)) >> .sorted(entryComparator) >> .forEachOrdered(validator); >> return validator.isValidated(); >> } catch (IOException e) { >> error(formatMsg2("error.validator.jarfile.exception", fname, >> e.getMessage())); >> return false; >> } catch (NumberFormatException e) { >> error(formatMsg("error.validator.bad.entry.name", >> e.getMessage())); >> return false; >> } >> >> Moreover what is the reason to have an external loop over the jar's entries? >> Even if we will use several different filter sets in future it would be >> better to use it as optional parameter for Validator. >> > I mean that the jar file entries iteration code ("580 > jf.stream().filter(e.... ") ... 588 }); ) > processing should be a part of "Validator”.
Yes, I know what you meant. I think it’s fine where it is, but I do like your suggestion to make Validator a Consumer and I’ll be releasing an updated webrev soon, with that change and the changes you recommended for the tests. Thanks for the ideas!
