On Tue, Jul 12, 2016 at 2:50 PM, Claes Redestad <claes.redes...@oracle.com> wrote: > > > On 2016-07-11 18:18, Volker Simonis wrote: >> >> Hi, >> >> here comes a new version of this change. I've tried to address most of >> the concerns/suggestions: >> >> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/ >> > > Looks good. As I'm currently obsessing about startup performance, I did > wish we could rely on (and fix, as a separate issue) the validation > done in build scripts, but there's no real harm in doing proper > validation in runtime as long as we avoid using regex here. >
Yes, I think the new checks should be quite cheap. >> >> - Finally, I didn't understood the comment about 'List.of might be >> preferred over Arrays.asList' so I didn't change that. > > > Oh, I meant that new List.of(...) can be used as a shorthand for > Arrays.asList(...) in the test. No real difference, just one less > import and a bit cleaner code in my opinion, so please ignore it at > this point. > Ah, I wasn't aware of the new method. Thanks, Volker > Thanks! > > /Claes > > >> >> OK to push now (before it gets really over-engineered :) >> >> Thank you and best regards, >> Volker >> >> On Thu, Jul 7, 2016 at 9:54 PM, Mandy Chung <mandy.ch...@oracle.com> >> wrote: >>> >>> Hi Volker, >>> >>> Thanks for adding a new test for it. >>> >>> Nit: can you wrap the long lines. >>> >>> As for the bad version, one possible change is to add assert >>> Runtime.Version.parse(versionNumber) in parseVersionNumbers method and add >>> -esa in @run tag. >>> >>> It’d be good to convert this to testng test where the dataprovider can >>> show the input version and expected returned list. >>> >>> Mandy >>> >>>> On Jul 7, 2016, at 6:59 AM, Volker Simonis <volker.simo...@gmail.com> >>>> wrote: >>>> >>>> Hi, >>>> >>>> can I please have a review for the following change which makes >>>> VersionProps.versionNumbers() testable and the corresponding >>>> regression test: >>>> >>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/ >>>> https://bugs.openjdk.java.net/browse/JDK-8160564 >>>> >>>> During the review for "8160457: VersionProps.versionNumbers() is >>>> broken" it was suggested to refactor VersionProps.versionNumbers() in >>>> order to make it testable by a regression test. This change does >>>> exactly that. It extracts the implementation of >>>> VersionProps.versionNumbers() into a new method >>>> parseVersionNumbers(String versionNumber) which can be tested from the >>>> associated regression test. >>>> >>>> There are still two points to notice: >>>> >>>> - VersionProps.versionNumbers() deliberately doesn't check for badly >>>> formatted version strings because it is assumed it will only get valid >>>> input (see discussion for "JDK-8160000: Runtime.version() cause >>>> startup regressions" at [2]). So we can currently only check that >>>> VersionProps.versionNumbers() accepts all kind of valid version >>>> strings. >>>> >>>> - I was a little bit surprised that I could reflectively access and >>>> execute java.lang.VersionProps.parseVersionNumbers() where both the >>>> class and the method are package private. Maybe this is related to >>>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a >>>> Jigsaw expert, I'd be graceful to anybody explaining me why this is >>>> still so easily possible with Jigsaw? >>>> >>>> Thank you and best regards, >>>> Volker >>>> >>>> >>>> [1] >>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html >>>> [2] https://bugs.openjdk.java.net/browse/JDK-8160000 >>>> [3] >>>> http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes >>> >>> >
