From 2e846616c47529635959c638ff212a72c5be645f Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Wed, 13 Mar 2019 16:08:09 -0700 Subject: [PATCH] ICU-20499 Fixing code path for plural form in MutablePatternModifier. --- icu4c/source/i18n/number_compact.cpp | 2 +- icu4c/source/i18n/number_longnames.cpp | 6 ++---- icu4c/source/i18n/number_patternmodifier.cpp | 18 +++++++----------- icu4c/source/i18n/number_patternmodifier.h | 2 +- icu4c/source/i18n/number_utils.h | 17 +++++++++++++++++ icu4c/source/test/intltest/numbertest.h | 1 + icu4c/source/test/intltest/numbertest_api.cpp | 9 +++++++++ .../intltest/numbertest_patternmodifier.cpp | 4 ++-- icu4c/source/test/intltest/numfmtst.cpp | 14 ++++++++++++++ icu4c/source/test/intltest/numfmtst.h | 1 + .../ibm/icu/impl/number/LongNameHandler.java | 6 ++---- .../impl/number/MutablePatternModifier.java | 13 ++++--------- .../com/ibm/icu/impl/number/RoundingUtils.java | 14 ++++++++++++++ .../com/ibm/icu/number/CompactNotation.java | 1 - .../icu/dev/test/format/NumberFormatTest.java | 13 ++++++++++++- .../test/number/NumberFormatterApiTest.java | 15 ++++++++++++--- 16 files changed, 99 insertions(+), 37 deletions(-) diff --git a/icu4c/source/i18n/number_compact.cpp b/icu4c/source/i18n/number_compact.cpp index cbcfb679ebf..f330251be38 100644 --- a/icu4c/source/i18n/number_compact.cpp +++ b/icu4c/source/i18n/number_compact.cpp @@ -297,7 +297,7 @@ void CompactHandler::processQuantity(DecimalQuantity &quantity, MicroProps &micr for (; i < precomputedModsLength; i++) { const CompactModInfo &info = precomputedMods[i]; if (u_strcmp(patternString, info.patternString) == 0) { - info.mod->applyToMicros(micros, quantity); + info.mod->applyToMicros(micros, quantity, status); break; } } diff --git a/icu4c/source/i18n/number_longnames.cpp b/icu4c/source/i18n/number_longnames.cpp index cfea339ef4d..0cd160042a4 100644 --- a/icu4c/source/i18n/number_longnames.cpp +++ b/icu4c/source/i18n/number_longnames.cpp @@ -313,10 +313,8 @@ void LongNameHandler::multiSimpleFormatsToModifiers(const UnicodeString *leadFor void LongNameHandler::processQuantity(DecimalQuantity &quantity, MicroProps µs, UErrorCode &status) const { parent->processQuantity(quantity, micros, status); - // TODO: Avoid the copy here? - DecimalQuantity copy(quantity); - micros.rounder.apply(copy, status); - micros.modOuter = &fModifiers[utils::getStandardPlural(rules, copy)]; + StandardPlural::Form pluralForm = utils::getPluralSafe(micros.rounder, rules, quantity, status); + micros.modOuter = &fModifiers[pluralForm]; } const Modifier* LongNameHandler::getModifier(int8_t /*signum*/, StandardPlural::Form plural) const { diff --git a/icu4c/source/i18n/number_patternmodifier.cpp b/icu4c/source/i18n/number_patternmodifier.cpp index 49ef8148741..75de439f3ed 100644 --- a/icu4c/source/i18n/number_patternmodifier.cpp +++ b/icu4c/source/i18n/number_patternmodifier.cpp @@ -127,18 +127,16 @@ ImmutablePatternModifier::ImmutablePatternModifier(AdoptingModifierStore* pm, co void ImmutablePatternModifier::processQuantity(DecimalQuantity& quantity, MicroProps& micros, UErrorCode& status) const { parent->processQuantity(quantity, micros, status); - applyToMicros(micros, quantity); + applyToMicros(micros, quantity, status); } -void ImmutablePatternModifier::applyToMicros(MicroProps& micros, DecimalQuantity& quantity) const { +void ImmutablePatternModifier::applyToMicros( + MicroProps& micros, const DecimalQuantity& quantity, UErrorCode& status) const { if (rules == nullptr) { micros.modMiddle = pm->getModifierWithoutPlural(quantity.signum()); } else { - // TODO: Fix this. Avoid the copy. - DecimalQuantity copy(quantity); - copy.roundToInfinity(); - StandardPlural::Form plural = utils::getStandardPlural(rules, copy); - micros.modMiddle = pm->getModifier(quantity.signum(), plural); + StandardPlural::Form pluralForm = utils::getPluralSafe(micros.rounder, rules, quantity, status); + micros.modMiddle = pm->getModifier(quantity.signum(), pluralForm); } } @@ -164,10 +162,8 @@ void MutablePatternModifier::processQuantity(DecimalQuantity& fq, MicroProps& mi // This method needs to be const because it overrides a const method in the parent class. auto nonConstThis = const_cast(this); if (needsPlurals()) { - // TODO: Fix this. Avoid the copy. - DecimalQuantity copy(fq); - micros.rounder.apply(copy, status); - nonConstThis->setNumberProperties(fq.signum(), utils::getStandardPlural(fRules, copy)); + StandardPlural::Form pluralForm = utils::getPluralSafe(micros.rounder, fRules, fq, status); + nonConstThis->setNumberProperties(fq.signum(), pluralForm); } else { nonConstThis->setNumberProperties(fq.signum(), StandardPlural::Form::COUNT); } diff --git a/icu4c/source/i18n/number_patternmodifier.h b/icu4c/source/i18n/number_patternmodifier.h index 675ea70c47a..27e293b64ce 100644 --- a/icu4c/source/i18n/number_patternmodifier.h +++ b/icu4c/source/i18n/number_patternmodifier.h @@ -46,7 +46,7 @@ class U_I18N_API ImmutablePatternModifier : public MicroPropsGenerator, public U void processQuantity(DecimalQuantity&, MicroProps& micros, UErrorCode& status) const U_OVERRIDE; - void applyToMicros(MicroProps& micros, DecimalQuantity& quantity) const; + void applyToMicros(MicroProps& micros, const DecimalQuantity& quantity, UErrorCode& status) const; const Modifier* getModifier(int8_t signum, StandardPlural::Form plural) const; diff --git a/icu4c/source/i18n/number_utils.h b/icu4c/source/i18n/number_utils.h index 05b8ec02a52..203dec6d83b 100644 --- a/icu4c/source/i18n/number_utils.h +++ b/icu4c/source/i18n/number_utils.h @@ -124,6 +124,23 @@ inline StandardPlural::Form getStandardPlural(const PluralRules *rules, } } +/** + * Computes the plural form after copying the number and applying rounding rules. + */ +inline StandardPlural::Form getPluralSafe( + const RoundingImpl& rounder, + const PluralRules* rules, + const DecimalQuantity& dq, + UErrorCode& status) { + // TODO(ICU-20500): Avoid the copy? + DecimalQuantity copy(dq); + rounder.apply(copy, status); + if (U_FAILURE(status)) { + return StandardPlural::Form::OTHER; + } + return getStandardPlural(rules, copy); +} + } // namespace utils } // namespace impl diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h index c07fe9e37ec..e1c90a75a85 100644 --- a/icu4c/source/test/intltest/numbertest.h +++ b/icu4c/source/test/intltest/numbertest.h @@ -90,6 +90,7 @@ class NumberFormatterApiTest : public IntlTestWithFieldPosition { CurrencyUnit CAD; CurrencyUnit ESP; CurrencyUnit PTE; + CurrencyUnit RON; MeasureUnit METER; MeasureUnit DAY; diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index f27292ff6e8..902d27f1271 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -32,6 +32,7 @@ NumberFormatterApiTest::NumberFormatterApiTest(UErrorCode& status) CAD(u"CAD", status), ESP(u"ESP", status), PTE(u"PTE", status), + RON(u"RON", status), FRENCH_SYMBOLS(Locale::getFrench(), status), SWISS_SYMBOLS(Locale("de-CH"), status), MYANMAR_SYMBOLS(Locale("my"), status) { @@ -788,6 +789,14 @@ void NumberFormatterApiTest::unitCurrency() { Locale("pt-PT"), 444444.55, u"444,444$55 PTE"); + + assertFormatSingle( + u"Plural form depending on visible digits (ICU-20499)", + u"currency/RON unit-width-full-name", + NumberFormatter::with().unit(RON).unitWidth(UNUM_UNIT_WIDTH_FULL_NAME), + Locale("ro-RO"), + 24, + u"24,00 lei românești"); } void NumberFormatterApiTest::unitPercent() { diff --git a/icu4c/source/test/intltest/numbertest_patternmodifier.cpp b/icu4c/source/test/intltest/numbertest_patternmodifier.cpp index 2156c9b435e..ba382a65615 100644 --- a/icu4c/source/test/intltest/numbertest_patternmodifier.cpp +++ b/icu4c/source/test/intltest/numbertest_patternmodifier.cpp @@ -119,7 +119,7 @@ void PatternModifierTest::testPatternWithNoPlaceholder() { return; } DecimalQuantity quantity; - imod->applyToMicros(micros, quantity); + imod->applyToMicros(micros, quantity, status); micros.modMiddle->apply(nsb, 1, 4, status); assertSuccess("Spot 7", status); assertEquals("Safe Path", u"xabcy", nsb.toUnicodeString()); @@ -151,7 +151,7 @@ void PatternModifierTest::testMutableEqualsImmutable() { NumberStringBuilder nsb2; MicroProps micros2; LocalPointer immutable(mod.createImmutable(status)); - immutable->applyToMicros(micros2, fq); + immutable->applyToMicros(micros2, fq, status); micros2.modMiddle->apply(nsb2, 0, 0, status); assertSuccess("Spot 4", status); diff --git a/icu4c/source/test/intltest/numfmtst.cpp b/icu4c/source/test/intltest/numfmtst.cpp index 4625b130164..7dd66e580a1 100644 --- a/icu4c/source/test/intltest/numfmtst.cpp +++ b/icu4c/source/test/intltest/numfmtst.cpp @@ -229,6 +229,7 @@ void NumberFormatTest::runIndexedTest( int32_t index, UBool exec, const char* &n TESTCASE_AUTO(Test20348_CurrencyPrefixOverride); TESTCASE_AUTO(Test20358_GroupingInPattern); TESTCASE_AUTO(Test13731_DefaultCurrency); + TESTCASE_AUTO(Test20499_CurrencyVisibleDigitsPlural); TESTCASE_AUTO_END; } @@ -9608,4 +9609,17 @@ void NumberFormatTest::Test13731_DefaultCurrency() { } } +void NumberFormatTest::Test20499_CurrencyVisibleDigitsPlural() { + IcuTestErrorCode status(*this, "Test20499_CurrencyVisibleDigitsPlural"); + LocalPointer nf(NumberFormat::createInstance( + "ro-RO", UNumberFormatStyle::UNUM_CURRENCY_PLURAL, status), status); + const char16_t* expected = u"24,00 lei românești"; + for (int32_t i=0; i<5; i++) { + UnicodeString actual; + nf->format(24, actual, status); + assertEquals(UnicodeString(u"iteration ") + Int64ToUnicodeString(i), + expected, actual); + } +} + #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/test/intltest/numfmtst.h b/icu4c/source/test/intltest/numfmtst.h index b62cde5523f..7dbd31c2df6 100644 --- a/icu4c/source/test/intltest/numfmtst.h +++ b/icu4c/source/test/intltest/numfmtst.h @@ -295,6 +295,7 @@ class NumberFormatTest: public CalendarTimeZoneTest { void Test20348_CurrencyPrefixOverride(); void Test20358_GroupingInPattern(); void Test13731_DefaultCurrency(); + void Test20499_CurrencyVisibleDigitsPlural(); private: UBool testFormattableAsUFormattable(const char *file, int line, Formattable &f); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/LongNameHandler.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/LongNameHandler.java index e31ea8b18a8..c0c6a74cb73 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/LongNameHandler.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/LongNameHandler.java @@ -290,10 +290,8 @@ public class LongNameHandler implements MicroPropsGenerator, ModifierStore { @Override public MicroProps processQuantity(DecimalQuantity quantity) { MicroProps micros = parent.processQuantity(quantity); - // TODO: Avoid the copy here? - DecimalQuantity copy = quantity.createCopy(); - micros.rounder.apply(copy); - micros.modOuter = modifiers.get(copy.getStandardPlural(rules)); + StandardPlural pluralForm = RoundingUtils.getPluralSafe(micros.rounder, rules, quantity); + micros.modOuter = modifiers.get(pluralForm); return micros; } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MutablePatternModifier.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MutablePatternModifier.java index 4cdde30d78c..b1c1e9a0ee7 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MutablePatternModifier.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MutablePatternModifier.java @@ -243,11 +243,8 @@ public class MutablePatternModifier implements Modifier, SymbolProvider, MicroPr if (rules == null) { micros.modMiddle = pm.getModifierWithoutPlural(quantity.signum()); } else { - // TODO: Fix this. Avoid the copy. - DecimalQuantity copy = quantity.createCopy(); - copy.roundToInfinity(); - StandardPlural plural = copy.getStandardPlural(rules); - micros.modMiddle = pm.getModifier(quantity.signum(), plural); + StandardPlural pluralForm = RoundingUtils.getPluralSafe(micros.rounder, rules, quantity); + micros.modMiddle = pm.getModifier(quantity.signum(), pluralForm); } } @@ -273,10 +270,8 @@ public class MutablePatternModifier implements Modifier, SymbolProvider, MicroPr public MicroProps processQuantity(DecimalQuantity fq) { MicroProps micros = parent.processQuantity(fq); if (needsPlurals()) { - // TODO: Fix this. Avoid the copy. - DecimalQuantity copy = fq.createCopy(); - micros.rounder.apply(copy); - setNumberProperties(fq.signum(), copy.getStandardPlural(rules)); + StandardPlural pluralForm = RoundingUtils.getPluralSafe(micros.rounder, rules, fq); + setNumberProperties(fq.signum(), pluralForm); } else { setNumberProperties(fq.signum(), null); } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/RoundingUtils.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/RoundingUtils.java index 5921f60ff2d..0f35a6602f8 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/RoundingUtils.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/RoundingUtils.java @@ -6,7 +6,10 @@ import java.math.BigDecimal; import java.math.MathContext; import java.math.RoundingMode; +import com.ibm.icu.impl.StandardPlural; +import com.ibm.icu.number.Precision; import com.ibm.icu.number.Scale; +import com.ibm.icu.text.PluralRules; /** @author sffc */ public class RoundingUtils { @@ -219,4 +222,15 @@ public class RoundingUtils { return null; } } + + /** + * Computes the plural form after copying the number and applying rounding rules. + */ + public static StandardPlural getPluralSafe( + Precision rounder, PluralRules rules, DecimalQuantity dq) { + // TODO(ICU-20500): Avoid the copy? + DecimalQuantity copy = dq.createCopy(); + rounder.apply(copy); + return copy.getStandardPlural(rules); + } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/CompactNotation.java b/icu4j/main/classes/core/src/com/ibm/icu/number/CompactNotation.java index 04e0eb9ea90..9502dc30ef9 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/CompactNotation.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/CompactNotation.java @@ -128,7 +128,6 @@ public class CompactNotation extends Notation { magnitude = 0; micros.rounder.apply(quantity); } else { - // TODO: Revisit chooseMultiplierAndApply int multiplier = micros.rounder.chooseMultiplierAndApply(quantity, data); magnitude = quantity.isZero() ? 0 : quantity.getMagnitude(); magnitude -= multiplier; diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java index cc702950fe3..0c76bd002e8 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java @@ -4152,7 +4152,7 @@ public class NumberFormatTest extends TestFmwk { int fracForRoundIncr = 0; if (roundIncrUsed) { double testIncr = item.roundIncr; - for (; testIncr > (double)((int)testIncr); testIncr *= 10.0, fracForRoundIncr++); + for (; testIncr > ((int)testIncr); testIncr *= 10.0, fracForRoundIncr++); } int minInt = df.getMinimumIntegerDigits(); @@ -6614,4 +6614,15 @@ public class NumberFormatTest extends TestFmwk { assertEquals("currency", "XXX", nf.getCurrency().getCurrencyCode()); } } + + @Test + public void test20499_CurrencyVisibleDigitsPlural() { + ULocale locale = new ULocale("ro-RO"); + NumberFormat nf = NumberFormat.getInstance(locale, NumberFormat.PLURALCURRENCYSTYLE); + String expected = "24,00 lei românești"; + for (int i=0; i<5; i++) { + String actual = nf.format(24); + assertEquals("iteration " + i, expected, actual); + } + } } 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 3d655aff44f..9421097eca6 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 @@ -65,6 +65,7 @@ public class NumberFormatterApiTest { private static final Currency CAD = Currency.getInstance("CAD"); private static final Currency ESP = Currency.getInstance("ESP"); private static final Currency PTE = Currency.getInstance("PTE"); + private static final Currency RON = Currency.getInstance("RON"); @Test public void notationSimple() { @@ -728,7 +729,7 @@ public class NumberFormatterApiTest { // NOTE: This is a bit of a hack on CLDR's part. They set the currency symbol to U+200B (zero- // width space), and they set the decimal separator to the $ symbol. assertFormatSingle( - "Currency-dependent symbols (Test)", + "Currency-dependent symbols (Test Short)", "currency/PTE unit-width-short", NumberFormatter.with().unit(PTE).unitWidth(UnitWidth.SHORT), ULocale.forLanguageTag("pt-PT"), @@ -736,7 +737,7 @@ public class NumberFormatterApiTest { "444,444$55 \u200B"); assertFormatSingle( - "Currency-dependent symbols (Test)", + "Currency-dependent symbols (Test Narrow)", "currency/PTE unit-width-narrow", NumberFormatter.with().unit(PTE).unitWidth(UnitWidth.NARROW), ULocale.forLanguageTag("pt-PT"), @@ -744,12 +745,20 @@ public class NumberFormatterApiTest { "444,444$55 \u200B"); assertFormatSingle( - "Currency-dependent symbols (Test)", + "Currency-dependent symbols (Test ISO Code)", "currency/PTE unit-width-iso-code", NumberFormatter.with().unit(PTE).unitWidth(UnitWidth.ISO_CODE), ULocale.forLanguageTag("pt-PT"), 444444.55, "444,444$55 PTE"); + + assertFormatSingle( + "Plural form depending on visible digits (ICU-20499)", + "currency/RON unit-width-full-name", + NumberFormatter.with().unit(RON).unitWidth(UnitWidth.FULL_NAME), + ULocale.forLanguageTag("ro-RO"), + 24, + "24,00 lei românești"); } @Test