ICU-20499 Fixing code path for plural form in MutablePatternModifier.

diff --git a/icu4c/source/i18n/number_compact.cpp b/icu4c/source/i18n/number_compact.cpp
index cbcfb679ebf17f757fb84e33741b0e055302ac18..f330251be38c88563be9d470472ee1b1214e0a91 100644 (file)
--- 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 cfea339ef4db22c72fb10d54cf1b666478043994..0cd160042a46d330d7b054ba3dfd961403d0c707 100644 (file)
--- 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 &micros,
                                       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 49ef8148741f7863b1bbd9640319fc19efa2a57b..75de439f3ed2e16ec5dfccb572c0892075f07547 100644 (file)
--- 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<MutablePatternModifier*>(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 675ea70c47a0d0f5c732ea1ec2bcf9225a7efcd7..27e293b64ce50fa77c16b3227cf1b22d4852933d 100644 (file)
--- 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 05b8ec02a52b5f1e07fe979e559c17841dde36be..203dec6d83b92b0d808721957746cc2ef886a628 100644 (file)
--- 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 c07fe9e37ecf31ec6dde2b8e224e413fc55b2f8e..e1c90a75a85759c99c237925cf864f0acbf4d2e0 100644 (file)
--- 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 f27292ff6e887d33b39a36057028b6e2d60a3b8e..902d27f1271c5fe0e58f668fdd4c93f435ef0f50 100644 (file)
--- 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 2156c9b435eecd25f644021f20d8b39262107480..ba382a65615dd6c7efa92b08f62332f859a090c5 100644 (file)
--- 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<ImmutablePatternModifier> 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 4625b130164910001c2e22f9ed97669ab2889f4c..7dd66e580a118663ef42efafdf460146a1ec3217 100644 (file)
--- 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<NumberFormat> 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 b62cde5523f8d8dd61dbec032af56cbac6d16aa6..7dbd31c2df6d97eb941befcc963f417755d132a9 100644 (file)
--- 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 e31ea8b18a85786222f4133d07f81b4cb3aa1baa..c0c6a74cb73ba3985d0f9edf7e9cecf54bcf7fca 100644 (file)
--- 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 4cdde30d78cec5c3d18b44c923c476827885a061..b1c1e9a0ee71b1a1d0d12307403b4d57c026fdba 100644 (file)
--- 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 5921f60ff2deae8ac311ff284e296a91f9662d67..0f35a6602f8bb94c585325667450154e54066dac 100644 (file)
--- 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 04e0eb9ea909c211620a186d3c09862a192fb835..9502dc30ef9b7a70a6ab6225685da5fe8431c0ed 100644 (file)
--- 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 cc702950fe3d1f692197c9a691681f5a6753b387..0c76bd002e87d1b9ba3978fdc02f89da6eb7c024 100644 (file)
--- 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 3d655aff44fc08c9fb6c17bd611b24e8badb79e0..9421097eca61846fd4b8667cf926f6a83ca6c516 100644 (file)
--- 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