Hi Brian.
This looks good to me, thanks!
One minor comment. Here:
3990 if (digits > 0) {
3991 padWithZeros(buf, digits);
3992 } else {
3993 buf.append('0');
3994 }
I cannot really see how digits may be <= 0, so it seems to me it can be
safely replaced by just one line `padWithZeros(buf, digits);`.
Alternatively, the entire branch `if (signum == 0) {` can be removed
from smallToString (so that this method will only work with strictly
positive numbers), and done only for results[1] at line 4083 because it
is the only possible source of ZERO values in the process.
It also seems that the arithmetic in the very internal loop at 4005-4017
can be slightly optimized, but this probably can be left for another
enhancement.
With kind regards,
Ivan
On 8/23/19 10:11 AM, Brian Burkhalter wrote:
Hi Ivan,
On Aug 22, 2019, at 1:24 PM, Ivan Gerasimov
<ivan.gerasi...@oracle.com <mailto:ivan.gerasi...@oracle.com>> wrote:
I have a few further suggestions how to simplify the code.
[…]
Those are all good suggestions: thanks!
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).
This patch has a problem:
4023 padWithZeros(buf, digits - s.length() + (numGroups - 1) *
digitsPerLong[radix]);
It is missing parentheses:
4023 padWithZeros(buf, digits - (s.length() + (numGroups - 1)
* digitsPerLong[radix]));
I was unable to find this by debugging but implemented your
suggestions by hand and then found it by diff-ing the patches. My
updated versions with your suggestions incorporated are: [1] delta,
[2] absolute. I also increased the size of some of the values tested
at line 798 of BigIntegerTest.
Thanks,
Brian
[1] http://cr.openjdk.java.net/~bpb/8229845/webrev.03-04/
[2] http://cr.openjdk.java.net/~bpb/8229845/webrev.04/
--
With kind regards,
Ivan Gerasimov
