On Tue, Jul 12, 2016 at 4:27 AM, Mandy Chung <mandy.ch...@oracle.com> wrote: > >> 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.
Please find the new webrev at: http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/ >I suggest to combine the check and Integer.parseInt in a single method (e.g. >“numberAt” or some other better method name). > Done. > 71 if (index - prevIndex > 1 && > > may read better as "index > prevIndex” > That's actually not the same. I would have to write "index > prevIndex + 1" which I think isn't much better. > 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. > Done. > 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. > Yes, that's much clearer. Thanks. Any other comments? Regards, Volker
