> On Jul 12, 2016, at 12:18 AM, Volker Simonis <volker.simo...@gmail.com> 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/
>
This version looks okay in general. I suggest to combine the check and
Integer.parseInt in a single method (e.g. “numberAt” or some other better
method name).
71 if (index - prevIndex > 1 &&
may read better as "index > prevIndex”
The new comment can simply say “This method is used by regression tests” that
should do it. I would not mention the test name in the source since
refactoring may make it outdated.
In the new test,
92 if (error) {
93 throw new Exception(invalidVersions[i] +
94 " not recognized as invalid by
VersionProps.parseVersionNumbers()");
95 }
An alternative to the above check, it can throw an exception immediate after
line 82 when parseVersionNumbers.invoke succeeds.
Mandy
