On Tue, 15 Apr 2025 14:43:56 GMT, Johannes Graham <[email protected]> wrote:
>> The `DigitList` class used in `DecimalFormat` does not reset the `data`
>> array in its clone method. This can cause interference when clones are used
>> concurrently.
>
> Johannes Graham has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - fix test summary
> - add test
Thanks for the fix, it looks good. Left some minor comments. I will run tiers
1-3 with your change.
src/java.base/share/classes/java/text/DigitList.java line 728:
> 726: System.arraycopy(digits, 0, newDigits, 0, digits.length);
> 727: other.digits = newDigits;
> 728: other.data = null;
We don't have to copy the data _array_ unlike the _digits_ array (above)
because it can just be reset during the next `getDataChars` call, but a comment
as to why might be helpful.
test/jdk/java/text/Format/DecimalFormat/CloneTest.java line 27:
> 25: * @test
> 26: * @bug 8354522
> 27: * @summary Check for cloning interference
It will probably be good to mention somewhere that this test/fix addresses the
issue of the same _data_ array reference being shared amongst DigitList clones.
test/jdk/java/text/Format/DecimalFormat/CloneTest.java line 48:
> 46: AtomicInteger mismatchCount = new AtomicInteger(0);
> 47: DecimalFormat df = new DecimalFormat("#");
> 48: String _ = df.format(Math.PI); // initial use of formatter
We should probably comment the importance of this line, as without it the test
will pass without the fix. (It sets the _data_ array to a non null value).
test/jdk/java/text/Format/DecimalFormat/CloneTest.java line 54:
> 52: DecimalFormat threadDf = (DecimalFormat) df.clone();
> 53: Runnable task = () -> {
> 54: for (int j = 0; j < 1000000; j++) {
We should probably make this test less costly by decreasing some values, I
don't the bug requires that many iterations to be exposed. (Without the fix and
the `break` statement in the test, `mismatchCount` gets up into the tens of
thousands on my machine.)
-------------
PR Review: https://git.openjdk.org/jdk/pull/24598#pullrequestreview-2769119077
PR Review Comment: https://git.openjdk.org/jdk/pull/24598#discussion_r2045055262
PR Review Comment: https://git.openjdk.org/jdk/pull/24598#discussion_r2045062899
PR Review Comment: https://git.openjdk.org/jdk/pull/24598#discussion_r2045083581
PR Review Comment: https://git.openjdk.org/jdk/pull/24598#discussion_r2045060617
