ICU-21134 Copy additional data when toNumberFormatter is used

See #1156

diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp
index 9cdb8b7156e614c0723073107ac132e2247522cf..da8cc7ed17833d9c0e72d66da44116c6a75cc97d 100644 (file)
--- a/icu4c/source/i18n/number_fluent.cpp
+++ b/icu4c/source/i18n/number_fluent.cpp
@@ -13,6 +13,7 @@
 #include "number_asformat.h"
 #include "number_utils.h"
 #include "number_utypes.h"
+#include "number_mapper.h"
 #include "util.h"
 #include "fphdlimp.h"
 
@@ -400,7 +401,8 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(const LNF& other)
 
 LocalizedNumberFormatter::LocalizedNumberFormatter(const NFS<LNF>& other)
         : NFS<LNF>(other) {
-    // No additional fields to assign (let call count and compiled formatter reset to defaults)
+    UErrorCode localStatus = U_ZERO_ERROR; // Can't bubble up the error
+    lnfCopyHelper(static_cast<const LNF&>(other), localStatus);
 }
 
 LocalizedNumberFormatter::LocalizedNumberFormatter(LocalizedNumberFormatter&& src) U_NOEXCEPT
@@ -408,38 +410,25 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(LocalizedNumberFormatter&& sr
 
 LocalizedNumberFormatter::LocalizedNumberFormatter(NFS<LNF>&& src) U_NOEXCEPT
         : NFS<LNF>(std::move(src)) {
-    // For the move operators, copy over the compiled formatter.
-    // Note: if the formatter is not compiled, call count information is lost.
-    if (static_cast<LNF&&>(src).fCompiled != nullptr) {
-        lnfMoveHelper(static_cast<LNF&&>(src));
-    }
+    lnfMoveHelper(std::move(static_cast<LNF&&>(src)));
 }
 
 LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(const LNF& other) {
     NFS<LNF>::operator=(static_cast<const NFS<LNF>&>(other));
-    // Reset to default values.
-    clear();
+    UErrorCode localStatus = U_ZERO_ERROR; // Can't bubble up the error
+    lnfCopyHelper(other, localStatus);
     return *this;
 }
 
 LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(LNF&& src) U_NOEXCEPT {
     NFS<LNF>::operator=(static_cast<NFS<LNF>&&>(src));
-    // For the move operators, copy over the compiled formatter.
-    // Note: if the formatter is not compiled, call count information is lost.
-    if (static_cast<LNF&&>(src).fCompiled != nullptr) {
-        // Formatter is compiled
-        lnfMoveHelper(static_cast<LNF&&>(src));
-    } else {
-        clear();
-    }
+    lnfMoveHelper(std::move(src));
     return *this;
 }
 
-void LocalizedNumberFormatter::clear() {
-    // Reset to default values.
+void LocalizedNumberFormatter::resetCompiled() {
     auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
     umtx_storeRelease(*callCount, 0);
-    delete fCompiled;
     fCompiled = nullptr;
 }
 
@@ -447,19 +436,56 @@ void LocalizedNumberFormatter::lnfMoveHelper(LNF&& src) {
     // Copy over the compiled formatter and set call count to INT32_MIN as in computeCompiled().
     // Don't copy the call count directly because doing so requires a loadAcquire/storeRelease.
     // The bits themselves appear to be platform-dependent, so copying them might not be safe.
-    auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
-    umtx_storeRelease(*callCount, INT32_MIN);
     delete fCompiled;
-    fCompiled = src.fCompiled;
-    // Reset the source object to leave it in a safe state.
-    auto* srcCallCount = reinterpret_cast<u_atomic_int32_t*>(src.fUnsafeCallCount);
-    umtx_storeRelease(*srcCallCount, 0);
-    src.fCompiled = nullptr;
+    if (src.fCompiled != nullptr) {
+        auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
+        umtx_storeRelease(*callCount, INT32_MIN);
+        fCompiled = src.fCompiled;
+        // Reset the source object to leave it in a safe state.
+        src.resetCompiled();
+    } else {
+        resetCompiled();
+    }
+
+    // Unconditionally move the warehouse
+    delete fWarehouse;
+    fWarehouse = src.fWarehouse;
+    src.fWarehouse = nullptr;
+}
+
+void LocalizedNumberFormatter::lnfCopyHelper(const LNF&, UErrorCode& status) {
+    // When copying, always reset the compiled formatter.
+    delete fCompiled;
+    resetCompiled();
+
+    // If MacroProps has a reference to AffixPatternProvider, we need to copy it.
+    // If MacroProps has a reference to PluralRules, copy that one, too.
+    delete fWarehouse;
+    if (fMacros.affixProvider || fMacros.rules) {
+        LocalPointer<DecimalFormatWarehouse> warehouse(new DecimalFormatWarehouse(), status);
+        if (U_FAILURE(status)) {
+            fWarehouse = nullptr;
+            return;
+        }
+        if (fMacros.affixProvider) {
+            warehouse->affixProvider.setTo(fMacros.affixProvider, status);
+            fMacros.affixProvider = &warehouse->affixProvider.get();
+        }
+        if (fMacros.rules) {
+            warehouse->rules.adoptInsteadAndCheckErrorCode(
+                new PluralRules(*fMacros.rules), status);
+            fMacros.rules = warehouse->rules.getAlias();
+        }
+        fWarehouse = warehouse.orphan();
+    } else {
+        fWarehouse = nullptr;
+    }
 }
 
 
 LocalizedNumberFormatter::~LocalizedNumberFormatter() {
     delete fCompiled;
+    delete fWarehouse;
 }
 
 LocalizedNumberFormatter::LocalizedNumberFormatter(const MacroProps& macros, const Locale& locale) {

diff --git a/icu4c/source/i18n/number_mapper.h b/icu4c/source/i18n/number_mapper.h
index d18b8b3c438cf42c48a288337ec51c57cbd6c49d..9ecd776b3b4795512159c3e60c3b2c32cbb81c50 100644 (file)
--- a/icu4c/source/i18n/number_mapper.h
+++ b/icu4c/source/i18n/number_mapper.h
@@ -136,6 +136,16 @@ class AutoAffixPatternProvider {
         }
     }
 
+    inline void setTo(const AffixPatternProvider* provider, UErrorCode& status) {
+        if (auto ptr = dynamic_cast<const PropertiesAffixPatternProvider*>(provider)) {
+            propertiesAPP = *ptr;
+        } else if (auto ptr = dynamic_cast<const CurrencyPluralInfoAffixProvider*>(provider)) {
+            currencyPluralInfoAPP = *ptr;
+        } else {
+            status = U_INTERNAL_PROGRAM_ERROR;
+        }
+    }
+
     inline const AffixPatternProvider& get() const {
       if (!currencyPluralInfoAPP.isBogus()) {
         return currencyPluralInfoAPP;
@@ -153,9 +163,9 @@ class AutoAffixPatternProvider {
 /**
  * A struct for ownership of a few objects needed for formatting.
  */
-struct DecimalFormatWarehouse {
+struct DecimalFormatWarehouse : public UMemory {
     AutoAffixPatternProvider affixProvider;
-
+    LocalPointer<PluralRules> rules;
 };
 
 

diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h
index 615cf49f7b7539ee501a2a5af77e94e9802fc1b6..08f93cd6cfa9953ba503c3b123369713056985f5 100644 (file)
--- a/icu4c/source/i18n/unicode/numberformatter.h
+++ b/icu4c/source/i18n/unicode/numberformatter.h
@@ -157,6 +157,7 @@ struct RangeMacroProps;
 struct UFormattedNumberImpl;
 class MutablePatternModifier;
 class ImmutablePatternModifier;
+struct DecimalFormatWarehouse;
 
 /**
  * Used for NumberRangeFormatter and implemented in numrange_fluent.cpp.
@@ -2385,6 +2386,10 @@ class U_I18N_API LocalizedNumberFormatter
     const impl::NumberFormatterImpl* fCompiled {nullptr};
     char fUnsafeCallCount[8] {};  // internally cast to u_atomic_int32_t
 
+    // Owned pointer to a DecimalFormatWarehouse, used when copying a LocalizedNumberFormatter
+    // from a DecimalFormat.
+    const impl::DecimalFormatWarehouse* fWarehouse {nullptr};
+
     explicit LocalizedNumberFormatter(const NumberFormatterSettings<LocalizedNumberFormatter>& other);
 
     explicit LocalizedNumberFormatter(NumberFormatterSettings<LocalizedNumberFormatter>&& src) U_NOEXCEPT;
@@ -2393,10 +2398,12 @@ class U_I18N_API LocalizedNumberFormatter
 
     LocalizedNumberFormatter(impl::MacroProps &&macros, const Locale &locale);
 
-    void clear();
+    void resetCompiled();
 
     void lnfMoveHelper(LocalizedNumberFormatter&& src);
 
+    void lnfCopyHelper(const LocalizedNumberFormatter& src, UErrorCode& status);
+
     /**
      * @return true if the compiled formatter is available.
      */

diff --git a/icu4c/source/test/intltest/numfmtst.cpp b/icu4c/source/test/intltest/numfmtst.cpp
index 5a26a7a5607f73c284d24d76bc8e6a26bdafb1bf..f6030a0b0c28c907e67a3b90868ca6a598582116 100644 (file)
--- a/icu4c/source/test/intltest/numfmtst.cpp
+++ b/icu4c/source/test/intltest/numfmtst.cpp
@@ -245,6 +245,7 @@ void NumberFormatTest::runIndexedTest( int32_t index, UBool exec, const char* &n
   TESTCASE_AUTO(Test13735_GroupingSizeGetter);
   TESTCASE_AUTO(Test13734_StrictFlexibleWhitespace);
   TESTCASE_AUTO(Test20961_CurrencyPluralPattern);
+  TESTCASE_AUTO(Test21134_ToNumberFormatter);
   TESTCASE_AUTO_END;
 }
 
@@ -9835,4 +9836,61 @@ void NumberFormatTest::Test20961_CurrencyPluralPattern() {
     }
 }
 
+void NumberFormatTest::Test21134_ToNumberFormatter() {
+    IcuTestErrorCode status(*this, "Test21134_ToNumberFormatter");
+    LocalizedNumberFormatter outer1;
+    LocalizedNumberFormatter outer2;
+    LocalPointer<LocalizedNumberFormatter> outer3;
+    {
+        // Case 1: new formatter object
+        DecimalFormat inner(u"a0b", {"en", status}, status);
+        if (auto ptr = inner.toNumberFormatter(status)) {
+            // Copy assignment
+            outer1 = *ptr;
+        } else {
+            status.errIfFailureAndReset();
+            return;
+        }
+    }
+    {
+        // Case 2: compiled formatter object (used at least 3 times)
+        DecimalFormat inner(u"c0d", {"en", status}, status);
+        UnicodeString dummy;
+        inner.format(100, dummy);
+        inner.format(100, dummy);
+        inner.format(100, dummy);
+        if (auto ptr = inner.toNumberFormatter(status)) {
+            // Copy assignment
+            outer2 = *ptr;
+        } else {
+            status.errIfFailureAndReset();
+            return;
+        }
+    }
+    {
+        // Case 3: currency plural info (different code path)
+        LocalPointer<DecimalFormat> inner(static_cast<DecimalFormat*>(
+            DecimalFormat::createInstance("en-US", UNUM_CURRENCY_PLURAL, status)));
+        if (auto ptr = inner->toNumberFormatter(status)) {
+            // Copy constructor
+            outer3.adoptInsteadAndCheckErrorCode(new LocalizedNumberFormatter(*ptr), status);
+        } else {
+            status.errIfFailureAndReset();
+            return;
+        }
+    }
+    auto result1 = outer1.formatDouble(99, status);
+    assertEquals("Using NumberFormatter from DecimalFormat, new version",
+        u"a99b",
+        result1.toTempString(status));
+    auto result2 = outer2.formatDouble(99, status);
+    assertEquals("Using NumberFormatter from DecimalFormat, compiled version",
+        u"c99d",
+        result2.toTempString(status));
+    auto result3 = outer3->formatDouble(99, status);
+    assertEquals("Using NumberFormatter from DecimalFormat, compiled version",
+        u"99.00 US dollars",
+        result3.toTempString(status));
+}
+
 #endif /* #if !UCONFIG_NO_FORMATTING */

diff --git a/icu4c/source/test/intltest/numfmtst.h b/icu4c/source/test/intltest/numfmtst.h
index 068ad589344f2841011557cc18e438943e28b027..bb8a1e486d40cd60d8d817ee6e1a854d49ea3414 100644 (file)
--- a/icu4c/source/test/intltest/numfmtst.h
+++ b/icu4c/source/test/intltest/numfmtst.h
@@ -301,6 +301,7 @@ class NumberFormatTest: public CalendarTimeZoneTest {
     void Test13735_GroupingSizeGetter();
     void Test13734_StrictFlexibleWhitespace();
     void Test20961_CurrencyPluralPattern();
+    void Test21134_ToNumberFormatter();
 
  private:
     UBool testFormattableAsUFormattable(const char *file, int line, Formattable &f);