Thank you Brian, this looks much better!
I have a few further suggestions how to simplify the code.
1)
If this BigInteger is negative, it is negated in two places: @ 3943 and
either @ 4003 or @ 4061.
It is better to keep abs value at the very beginning and make private
toString(...) and smallToString() to only accept non-negative numbers.
2)
Once the 1) is done, it is possible to remove lines 3948-3954, and
always call private toString(...), which will immediately delegate to
smallToString() if necessary. This is just to make the code shorter.
3)
In padWithZeros(), the condition check `if (digits > 0 && len < digits)
{` is not necessary.
Actually, I would suggest to make a single argument, say,
numLeadingZeros (in the current code it is `m`).
4)
I think that logic with making the argument `digits` == -1 for the first
chunk of digits is over-complication: `digits` can always be safely set
to 1 in the initial call to the private toString() or smallToString().
Then the variable atBeginning is not needed at all.
5)
If 3) and 4) are done, the two lines 3988 and 3989 can be reduced to
`padWithZeros(buf, digits);`
Here's the patch, which illustrates all the suggestions above:
http://cr.openjdk.java.net/~igerasim/8229845/00/webrev/
(I have only run the tests from test/jdk/java/math/BigInteger to verify it).
With kind regards,
Ivan
On 8/22/19 11:04 AM, Brian Burkhalter wrote:
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 <mailto: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 zero*s*.
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/
--
With kind regards,
Ivan Gerasimov
