From 3ff6627ce68e5849aff9129b416197a22676654a Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Thu, 28 May 2020 02:13:07 +0000 Subject: [PATCH] ICU-21134 Copy additional data when toNumberFormatter is used See #1156 --- icu4c/source/i18n/number_fluent.cpp | 78 ++++++++++++++------- icu4c/source/i18n/number_mapper.h | 14 +++- icu4c/source/i18n/unicode/numberformatter.h | 9 ++- icu4c/source/test/intltest/numfmtst.cpp | 58 +++++++++++++++ icu4c/source/test/intltest/numfmtst.h | 1 + 5 files changed, 131 insertions(+), 29 deletions(-) diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp index 9cdb8b7156..da8cc7ed17 100644 --- 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& other) : NFS(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(other), localStatus); } LocalizedNumberFormatter::LocalizedNumberFormatter(LocalizedNumberFormatter&& src) U_NOEXCEPT @@ -408,38 +410,25 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(LocalizedNumberFormatter&& sr LocalizedNumberFormatter::LocalizedNumberFormatter(NFS&& src) U_NOEXCEPT : NFS(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(src).fCompiled != nullptr) { - lnfMoveHelper(static_cast(src)); - } + lnfMoveHelper(std::move(static_cast(src))); } LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(const LNF& other) { NFS::operator=(static_cast&>(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::operator=(static_cast&&>(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(src).fCompiled != nullptr) { - // Formatter is compiled - lnfMoveHelper(static_cast(src)); - } else { - clear(); - } + lnfMoveHelper(std::move(src)); return *this; } -void LocalizedNumberFormatter::clear() { - // Reset to default values. +void LocalizedNumberFormatter::resetCompiled() { auto* callCount = reinterpret_cast(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(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(src.fUnsafeCallCount); - umtx_storeRelease(*srcCallCount, 0); - src.fCompiled = nullptr; + if (src.fCompiled != nullptr) { + auto* callCount = reinterpret_cast(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 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 d18b8b3c43..9ecd776b3b 100644 --- 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(provider)) { + propertiesAPP = *ptr; + } else if (auto ptr = dynamic_cast(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 rules; }; diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 615cf49f7b..08f93cd6cf 100644 --- 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& other); explicit LocalizedNumberFormatter(NumberFormatterSettings&& src) U_NOEXCEPT; @@ -2393,10 +2398,12 @@ class U_I18N_API LocalizedNumberFormatter LocalizedNumberFormatter(impl::MacroProps &¯os, 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 5a26a7a560..f6030a0b0c 100644 --- 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 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 inner(static_cast( + 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 068ad58934..bb8a1e486d 100644 --- 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);