From 7b1bcdcb61642b2302ae46ff41885f97d1f37aff Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Tue, 14 Sep 2021 08:08:10 +0000 Subject: [PATCH] ICU-21668 Fix nickel rounding with away-from-zero rounding mode See #1814 --- icu4c/source/i18n/number_decimalquantity.cpp | 5 ++ icu4c/source/i18n/number_decimalquantity.h | 4 +- icu4c/source/test/intltest/numbertest.h | 2 +- icu4c/source/test/intltest/numbertest_api.cpp | 90 +++++++++++++++++-- .../number/DecimalQuantity_AbstractBCD.java | 11 ++- .../test/number/NumberFormatterApiTest.java | 84 ++++++++++++++++- 6 files changed, 185 insertions(+), 11 deletions(-) diff --git a/icu4c/source/i18n/number_decimalquantity.cpp b/icu4c/source/i18n/number_decimalquantity.cpp index 441d1682908..308c0e30e81 100644 --- a/icu4c/source/i18n/number_decimalquantity.cpp +++ b/icu4c/source/i18n/number_decimalquantity.cpp @@ -828,6 +828,7 @@ void DecimalQuantity::roundToMagnitude(int32_t magnitude, RoundingMode roundingM // Perform truncation if (position >= precision) { + U_ASSERT(trailingDigit == 0); setBcdToZero(); scale = magnitude; } else { @@ -845,6 +846,10 @@ void DecimalQuantity::roundToMagnitude(int32_t magnitude, RoundingMode roundingM // do not return: use the bubbling logic below } else { setDigitPos(0, 5); + // If the quantity was set to 0, we may need to restore a digit. + if (precision == 0) { + precision = 1; + } // compact not necessary: digit at position 0 is nonzero return; } diff --git a/icu4c/source/i18n/number_decimalquantity.h b/icu4c/source/i18n/number_decimalquantity.h index 818715462cd..107c09a96a5 100644 --- a/icu4c/source/i18n/number_decimalquantity.h +++ b/icu4c/source/i18n/number_decimalquantity.h @@ -433,7 +433,9 @@ class U_I18N_API DecimalQuantity : public IFixedDecimal, public UMemory { /** * Sets the digit in the BCD list. This method only sets the digit; it is the caller's - * responsibility to call {@link #compact} after setting the digit. + * responsibility to call {@link #compact} after setting the digit, and to ensure + * that the precision field is updated to reflect the correct number of digits if a + * nonzero digit is added to the decimal. * * @param position The position of the digit to pop, counted in BCD units from the least * significant digit. If outside the range supported by the implementation, an AssertionError diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h index 6a34185731a..309a42ae732 100644 --- a/icu4c/source/test/intltest/numbertest.h +++ b/icu4c/source/test/intltest/numbertest.h @@ -73,7 +73,7 @@ class NumberFormatterApiTest : public IntlTestWithFieldPosition { void roundingFigures(); void roundingFractionFigures(); void roundingOther(); - void roundingIncrementSkeleton(); + void roundingIncrementRegressionTest(); void grouping(); void padding(); void integerWidth(); diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 67713586e66..17207d96dd7 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -97,7 +97,7 @@ void NumberFormatterApiTest::runIndexedTest(int32_t index, UBool exec, const cha TESTCASE_AUTO(roundingFigures); TESTCASE_AUTO(roundingFractionFigures); TESTCASE_AUTO(roundingOther); - TESTCASE_AUTO(roundingIncrementSkeleton); + TESTCASE_AUTO(roundingIncrementRegressionTest); TESTCASE_AUTO(grouping); TESTCASE_AUTO(padding); TESTCASE_AUTO(integerWidth); @@ -3181,6 +3181,78 @@ void NumberFormatterApiTest::roundingOther() { u"0.000", u"0.000"); + assertFormatDescending( + u"Medium nickel increment with rounding mode ceiling (ICU-21668)", + u"precision-increment/50 rounding-mode-ceiling", + u"precision-increment/50 rounding-mode-ceiling", + NumberFormatter::with() + .precision(Precision::increment(50)) + .roundingMode(UNUM_ROUND_CEILING), + Locale::getEnglish(), + u"87,650", + u"8,800", + u"900", + u"100", + u"50", + u"50", + u"50", + u"50", + u"0"); + + assertFormatDescending( + u"Large nickel increment with rounding mode up (ICU-21668)", + u"precision-increment/5000 rounding-mode-up", + u"precision-increment/5000 rounding-mode-up", + NumberFormatter::with() + .precision(Precision::increment(5000)) + .roundingMode(UNUM_ROUND_UP), + Locale::getEnglish(), + u"90,000", + u"10,000", + u"5,000", + u"5,000", + u"5,000", + u"5,000", + u"5,000", + u"5,000", + u"0"); + + assertFormatDescending( + u"Large dime increment with rounding mode up (ICU-21668)", + u"precision-increment/10000 rounding-mode-up", + u"precision-increment/10000 rounding-mode-up", + NumberFormatter::with() + .precision(Precision::increment(10000)) + .roundingMode(UNUM_ROUND_UP), + Locale::getEnglish(), + u"90,000", + u"10,000", + u"10,000", + u"10,000", + u"10,000", + u"10,000", + u"10,000", + u"10,000", + u"0"); + + assertFormatDescending( + u"Large non-nickel increment with rounding mode up (ICU-21668)", + u"precision-increment/15000 rounding-mode-up", + u"precision-increment/15000 rounding-mode-up", + NumberFormatter::with() + .precision(Precision::increment(15000)) + .roundingMode(UNUM_ROUND_UP), + Locale::getEnglish(), + u"90,000", + u"15,000", + u"15,000", + u"15,000", + u"15,000", + u"15,000", + u"15,000", + u"15,000", + u"0"); + assertFormatDescending( u"Increment Resolving to Power of 10", u"precision-increment/0.010", @@ -3358,9 +3430,9 @@ void NumberFormatterApiTest::roundingOther() { u"5E-324"); } -/** Test for ICU-21654 */ -void NumberFormatterApiTest::roundingIncrementSkeleton() { - IcuTestErrorCode status(*this, "roundingIncrementSkeleton"); +/** Test for ICU-21654 and ICU-21668 */ +void NumberFormatterApiTest::roundingIncrementRegressionTest() { + IcuTestErrorCode status(*this, "roundingIncrementRegressionTest"); Locale locale = Locale::getEnglish(); for (int min_fraction_digits = 1; min_fraction_digits < 8; min_fraction_digits++) { @@ -3380,7 +3452,7 @@ void NumberFormatterApiTest::roundingIncrementSkeleton() { char message[256]; snprintf(message, 256, - "Precision::increment(%0.5f).withMinFraction(%d) '%s'\n", + "ICU-21654: Precision::increment(%0.5f).withMinFraction(%d) '%s'\n", increment, min_fraction_digits, skeleton.c_str()); @@ -3405,6 +3477,14 @@ void NumberFormatterApiTest::roundingIncrementSkeleton() { } } } + + auto increment = NumberFormatter::with() + .precision(Precision::increment(5000).withMinFraction(0)) + .roundingMode(UNUM_ROUND_UP) + .locale(Locale::getEnglish()) + .formatDouble(5.625, status) + .toString(status); + assertEquals("ICU-21668", u"5,000", increment); } void NumberFormatterApiTest::grouping() { diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_AbstractBCD.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_AbstractBCD.java index 25445bea159..e43e1ee5323 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_AbstractBCD.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_AbstractBCD.java @@ -924,6 +924,7 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity { // Perform truncation if (position >= precision) { + assert trailingDigit == 0; setBcdToZero(); scale = magnitude; } else { @@ -941,6 +942,10 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity { // do not return: use the bubbling logic below } else { setDigitPos(0, (byte) 5); + // If the quantity was set to 0, we may need to restore a digit. + if (precision == 0) { + precision = 1; + } // compact not necessary: digit at position 0 is nonzero return; } @@ -1165,8 +1170,10 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity { protected abstract byte getDigitPos(int position); /** - * Sets the digit in the BCD list. This method only sets the digit; it is the caller's responsibility - * to call {@link #compact} after setting the digit. + * Sets the digit in the BCD list. This method only sets the digit; it is the caller's + * responsibility to call {@link #compact} after setting the digit, and to ensure + * that the precision field is updated to reflect the correct number of digits if a + * nonzero digit is added to the decimal. * * @param position * The position of the digit to pop, counted in BCD units from the least significant diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java index 156c5d6a9a8..68817dbf13a 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java @@ -3166,6 +3166,78 @@ public class NumberFormatterApiTest extends TestFmwk { "0.000", "0.000"); + assertFormatDescending( + "Medium nickel increment with rounding mode ceiling (ICU-21668)", + "precision-increment/50 rounding-mode-ceiling", + "precision-increment/50 rounding-mode-ceiling", + NumberFormatter.with() + .precision(Precision.increment(new BigDecimal("50"))) + .roundingMode(RoundingMode.CEILING), + ULocale.ENGLISH, + "87,650", + "8,800", + "900", + "100", + "50", + "50", + "50", + "50", + "0"); + + assertFormatDescending( + "Large nickel increment with rounding mode up (ICU-21668)", + "precision-increment/5000 rounding-mode-up", + "precision-increment/5000 rounding-mode-up", + NumberFormatter.with() + .precision(Precision.increment(new BigDecimal("5000"))) + .roundingMode(RoundingMode.UP), + ULocale.ENGLISH, + "90,000", + "10,000", + "5,000", + "5,000", + "5,000", + "5,000", + "5,000", + "5,000", + "0"); + + assertFormatDescending( + "Large dime increment with rounding mode up (ICU-21668)", + "precision-increment/10000 rounding-mode-up", + "precision-increment/10000 rounding-mode-up", + NumberFormatter.with() + .precision(Precision.increment(new BigDecimal("10000"))) + .roundingMode(RoundingMode.UP), + ULocale.ENGLISH, + "90,000", + "10,000", + "10,000", + "10,000", + "10,000", + "10,000", + "10,000", + "10,000", + "0"); + + assertFormatDescending( + "Large non-nickel increment with rounding mode up (ICU-21668)", + "precision-increment/15000 rounding-mode-up", + "precision-increment/15000 rounding-mode-up", + NumberFormatter.with() + .precision(Precision.increment(new BigDecimal("15000"))) + .roundingMode(RoundingMode.UP), + ULocale.ENGLISH, + "90,000", + "15,000", + "15,000", + "15,000", + "15,000", + "15,000", + "15,000", + "15,000", + "0"); + assertFormatDescending( "Increment Resolving to Power of 10", "precision-increment/0.010", @@ -3334,7 +3406,7 @@ public class NumberFormatterApiTest extends TestFmwk { } @Test - public void roundingIncrementSkeleton() { + public void roundingIncrementRegressionTest() { ULocale locale = ULocale.ENGLISH; for (int min_fraction_digits = 1; min_fraction_digits < 8; min_fraction_digits++) { @@ -3357,7 +3429,7 @@ public class NumberFormatterApiTest extends TestFmwk { String skeleton = f.toSkeleton(); String message = String.format( - "Precision::increment(%.5f).withMinFraction(%d) '%s'\n", + "ICU-21654: Precision::increment(%.5f).withMinFraction(%d) '%s'\n", increment, min_fraction_digits, skeleton); @@ -3381,6 +3453,14 @@ public class NumberFormatterApiTest extends TestFmwk { } } } + + String increment = NumberFormatter.with() + .precision(Precision.increment(new BigDecimal("5000"))) + .roundingMode(RoundingMode.UP) + .locale(ULocale.ENGLISH) + .format(5.625) + .toString(); + assertEquals("ICU-21668", "5,000", increment); } @Test