Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
On Thu, 17 Feb 2022 07:32:36 GMT, Selikoff wrote: > Can I please be added as a Reviewer? I was the one who originally reported > this bug on 2/7/2022 via Oracle's web form. To be listed as a Reviewer, you will need to be a Reviewer of the JDK Project (https://openjdk.java.net/bylaws#reviewer). Instead, I tried to add you as a contributor in this PR, but failed as I wasn't aware `/contributor` command does not work after the PR is closed. Sorry. - PR: https://git.openjdk.java.net/jdk/pull/7412
Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
On Tue, 15 Feb 2022 22:31:47 GMT, Naoto Sato wrote: >> Fixing an issue in `CompactNumberFormat` which was caused by >> BigDecimal.divide() that incremented the number in the resulting format >> string. Also fixing some typos by taking this opportunity. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments Can I please be added as a Reviewer? I was the one who originally reported this bug on 2/7/2022 via Oracle's web form. - PR: https://git.openjdk.java.net/jdk/pull/7412
Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
On Tue, 15 Feb 2022 22:31:28 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/text/CompactNumberFormat.java line 595: >> >>> 593: divisor = (Long) divisors.get(++compactDataIndex); >>> 594: iPart = getIntegerPart(number, divisor); >>> 595: } >> >> This and the few lines surrounding it were duplicated. Might be worth >> considering consolidation. > > Right. In fact, I tried to converge them but ended up not to, as there are > slight differences for each number type. Yeah, that's fine. - PR: https://git.openjdk.java.net/jdk/pull/7412
Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
On Tue, 15 Feb 2022 22:31:47 GMT, Naoto Sato wrote: >> Fixing an issue in `CompactNumberFormat` which was caused by >> BigDecimal.divide() that incremented the number in the resulting format >> string. Also fixing some typos by taking this opportunity. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7412
Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
On Tue, 15 Feb 2022 21:59:42 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressing review comments
>
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 595:
>
>> 593: divisor = (Long) divisors.get(++compactDataIndex);
>> 594: iPart = getIntegerPart(number, divisor);
>> 595: }
>
> This and the few lines surrounding it were duplicated. Might be worth
> considering consolidation.
Right. In fact, I tried to converge them but ended up not to, as there are
slight differences for each number type.
> test/jdk/java/text/Format/CompactNumberFormat/TestCompactPatternsValidity.java
> line 106:
>
>> 104: {COMPACT_PATTERN8, List.of(new
>> BigInteger("223565686837667632"), new BigDecimal("12322456774334.89766"),
>> 3, 3456.78,
>> 105: new BigInteger(""), new
>> BigDecimal(".0"), 999_999),
>> 106: List.of("223566T", "12T", "30K", "3K", "1T", "1T",
>> "1M")},
>
> The case where 999_999_999 was formatted to 1000 million and now 1 billion
> may be added too.
Sure, added the 1 bil test case.
-
PR: https://git.openjdk.java.net/jdk/pull/7412
Re: RFR: 8281317: CompactNumberFormat displays 4-digit values when rounding to a new range [v2]
> Fixing an issue in `CompactNumberFormat` which was caused by > BigDecimal.divide() that incremented the number in the resulting format > string. Also fixing some typos by taking this opportunity. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7412/files - new: https://git.openjdk.java.net/jdk/pull/7412/files/8ea8ccfa..f6c86254 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7412=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7412=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7412.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7412/head:pull/7412 PR: https://git.openjdk.java.net/jdk/pull/7412
