On 11/23/2015 08:58 PM, John Rose wrote: > On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov <ivan.gerasi...@oracle.com > <mailto:ivan.gerasi...@oracle.com>> wrote: >> >> Though, it may be better to get yet another pair of eyes. >> >> One minor nit: In the tests, in the summary, it is written, "Test >> Integer.toString method*s*", but only one of the overloads is tested. > > Here's another nit in the tests. > This is supposed to "wiggle around" critical points, which I agree with. > But it only wiggles above: > > 39 while (base < Long.MAX_VALUE / 10) { > 40 for (int c = 1; c < 65536; c++) { > 41 buildAndTest(base + c); > 42 } > 43 base = (base == 0) ? 1 : base * 10; > 44 }
Yes, it *does* wiggle around: 71 test(sbN.toString(), -c); 72 test(sbP.toString(), c); This saves much testing time to build both "c" and "-c" representations. > You'll need to guard the call to buildAndTest to avoid negatives. It already does: 51 private static void buildAndTest(int c) { 52 if (c < 0) { 53 throw new IllegalArgumentException("Test bug: cannot handle negatives, " + c); 54 } > You could also start base at, say, 10000. I don't see a reason for this, why? The overlap in the test data is fine, if that makes test more understandable and therefore reliable. Thanks, -Aleksey