Hi Ivan,

Please see the delta [1] and absolute [2] webrevs.

> On Aug 21, 2019, at 7:38 PM, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
> 
> A couple of comments
> 
> 1)
> 3974         if (signum == 0) {
> 3975             buf.append('0');
> 3976             return;
> 3977         }
> 
> Shouldn't it be padded with zeroes, if digits > 0?
> If I'm not mistaken, it could happen if result[1] at the end of toString() 
> happens to be ZERO.

Good catch: you are absolutely correct.

> It that's true, then it may be a good idea to add a testcase for a number 
> with many trailing zeros in a string representation. Currently, 
> test/jdk/java/math/BigInteger/BigIntegerTest.java in stringConv() operates on 
> random numbers, which are unlikely to have huge number of trailing zeros.

I added testing this to stringConv(). The test fails without the most recent 
delta applied to the implementation.

> 2)
> 4120     /* zero is a string of NUM_ZEROS consecutive zeros. */
> 4121     private static final String zeros = "0".repeat(NUM_ZEROS);
> 
> Nit in the comment:  name of the constant should be zeros.

I changed the variable name to ZEROS as it is a constant.

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8229845/webrev.02-03/
[2] http://cr.openjdk.java.net/~bpb/8229845/webrev.03/

Reply via email to