From e59eb483143a659d13498b756c91cbbdeed2573c Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Sat, 21 Apr 2018 08:01:19 +0000 Subject: [PATCH] ICU-13634 Refactoring getPrefixSuffix methods. In ICU4C, the pattern modifier is now accessed directly. In ICU4J, they use the same detour through the pipeline code path as before with a TODO to improve to be closer to ICU4C. In addition, in both ICU4C and ICU4J, getPrefixSuffix now uses the compiled formatter when available. X-SVN-Rev: 41258 --- icu4c/source/i18n/decimfmt.cpp | 8 +-- icu4c/source/i18n/number_fluent.cpp | 52 +++++++++++-------- icu4c/source/i18n/number_formatimpl.cpp | 31 +++++++---- icu4c/source/i18n/number_formatimpl.h | 19 ++++--- icu4c/source/i18n/number_patternmodifier.cpp | 9 ++++ icu4c/source/i18n/number_patternmodifier.h | 2 + icu4c/source/i18n/unicode/numberformatter.h | 7 ++- .../impl/number/MutablePatternModifier.java | 11 ++++ .../icu/number/LocalizedNumberFormatter.java | 49 +++++++++++------ .../ibm/icu/number/NumberFormatterImpl.java | 29 ++++++++--- .../src/com/ibm/icu/text/DecimalFormat.java | 8 +-- .../icu/dev/test/format/NumberFormatTest.java | 2 +- 12 files changed, 157 insertions(+), 70 deletions(-) diff --git a/icu4c/source/i18n/decimfmt.cpp b/icu4c/source/i18n/decimfmt.cpp index febcaebbc14..b9b95077fc2 100644 --- a/icu4c/source/i18n/decimfmt.cpp +++ b/icu4c/source/i18n/decimfmt.cpp @@ -614,7 +614,7 @@ void DecimalFormat::setCurrencyPluralInfo(const CurrencyPluralInfo& info) { UnicodeString& DecimalFormat::getPositivePrefix(UnicodeString& result) const { ErrorCode localStatus; - fFormatter->getAffix(true, false, result, localStatus); + fFormatter->getAffixImpl(true, false, result, localStatus); return result; } @@ -626,7 +626,7 @@ void DecimalFormat::setPositivePrefix(const UnicodeString& newValue) { UnicodeString& DecimalFormat::getNegativePrefix(UnicodeString& result) const { ErrorCode localStatus; - fFormatter->getAffix(true, true, result, localStatus); + fFormatter->getAffixImpl(true, true, result, localStatus); return result; } @@ -638,7 +638,7 @@ void DecimalFormat::setNegativePrefix(const UnicodeString& newValue) { UnicodeString& DecimalFormat::getPositiveSuffix(UnicodeString& result) const { ErrorCode localStatus; - fFormatter->getAffix(false, false, result, localStatus); + fFormatter->getAffixImpl(false, false, result, localStatus); return result; } @@ -650,7 +650,7 @@ void DecimalFormat::setPositiveSuffix(const UnicodeString& newValue) { UnicodeString& DecimalFormat::getNegativeSuffix(UnicodeString& result) const { ErrorCode localStatus; - fFormatter->getAffix(false, true, result, localStatus); + fFormatter->getAffixImpl(false, true, result, localStatus); return result; } diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp index a22ab4de8ce..fe9105d65f9 100644 --- a/icu4c/source/i18n/number_fluent.cpp +++ b/icu4c/source/i18n/number_fluent.cpp @@ -640,6 +640,34 @@ LocalizedNumberFormatter::formatDecimalQuantity(const DecimalQuantity& dq, UErro } void LocalizedNumberFormatter::formatImpl(impl::UFormattedNumberData* results, UErrorCode& status) const { + if (computeCompiled(status)) { + fCompiled->apply(results->quantity, results->string, status); + } else { + NumberFormatterImpl::applyStatic(fMacros, results->quantity, results->string, status); + } +} + +void LocalizedNumberFormatter::getAffixImpl(bool isPrefix, bool isNegative, UnicodeString& result, + UErrorCode& status) const { + NumberStringBuilder string; + auto signum = static_cast(isNegative ? -1 : 1); + // Always return affixes for plural form OTHER. + static const StandardPlural::Form plural = StandardPlural::OTHER; + int32_t prefixLength; + if (computeCompiled(status)) { + prefixLength = fCompiled->getPrefixSuffix(signum, plural, string, status); + } else { + prefixLength = NumberFormatterImpl::getPrefixSuffixStatic(fMacros, signum, plural, string, status); + } + result.remove(); + if (isPrefix) { + result.append(string.toTempUnicodeString().tempSubStringBetween(0, prefixLength)); + } else { + result.append(string.toTempUnicodeString().tempSubStringBetween(prefixLength, string.length())); + } +} + +bool LocalizedNumberFormatter::computeCompiled(UErrorCode& status) const { // fUnsafeCallCount contains memory to be interpreted as an atomic int, most commonly // std::atomic. Since the type of atomic int is platform-dependent, we cast the // bytes in fUnsafeCallCount to u_atomic_int32_t, a typedef for the platform-dependent @@ -666,32 +694,14 @@ void LocalizedNumberFormatter::formatImpl(impl::UFormattedNumberData* results, U U_ASSERT(fCompiled == nullptr); const_cast(this)->fCompiled = compiled; umtx_storeRelease(*callCount, INT32_MIN); - compiled->apply(results->quantity, results->string, status); + return true; } else if (currentCount < 0) { // The data structure is already built; use it (fast path). U_ASSERT(fCompiled != nullptr); - fCompiled->apply(results->quantity, results->string, status); + return true; } else { // Format the number without building the data structure (slow path). - NumberFormatterImpl::applyStatic(fMacros, results->quantity, results->string, status); - } -} - -void LocalizedNumberFormatter::getAffix(bool isPrefix, bool isNegative, UnicodeString& result, - UErrorCode& status) const { - NumberStringBuilder nsb; - DecimalQuantity dq; - if (isNegative) { - dq.setToInt(-1); - } else { - dq.setToInt(1); - } - int prefixLength = NumberFormatterImpl::getPrefixSuffix(fMacros, dq, nsb, status); - result.remove(); - if (isPrefix) { - result.append(nsb.toTempUnicodeString().tempSubStringBetween(0, prefixLength)); - } else { - result.append(nsb.toTempUnicodeString().tempSubStringBetween(prefixLength, nsb.length())); + return false; } } diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index 885fd3fd3b1..10f327cf9a0 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -73,10 +73,11 @@ void NumberFormatterImpl::applyStatic(const MacroProps& macros, DecimalQuantity& impl.applyUnsafe(inValue, outString, status); } -int32_t NumberFormatterImpl::getPrefixSuffix(const MacroProps& macros, DecimalQuantity& inValue, - NumberStringBuilder& outString, UErrorCode& status) { +int32_t NumberFormatterImpl::getPrefixSuffixStatic(const MacroProps& macros, int8_t signum, + StandardPlural::Form plural, + NumberStringBuilder& outString, UErrorCode& status) { NumberFormatterImpl impl(macros, false, status); - return impl.getPrefixSuffixUnsafe(inValue, outString, status); + return impl.getPrefixSuffixUnsafe(signum, plural, outString, status); } // NOTE: C++ SPECIFIC DIFFERENCE FROM JAVA: @@ -101,16 +102,26 @@ void NumberFormatterImpl::applyUnsafe(DecimalQuantity& inValue, NumberStringBuil microsToString(fMicros, inValue, outString, status); } -int32_t -NumberFormatterImpl::getPrefixSuffixUnsafe(DecimalQuantity& inValue, NumberStringBuilder& outString, - UErrorCode& status) { +int32_t NumberFormatterImpl::getPrefixSuffix(int8_t signum, StandardPlural::Form plural, + NumberStringBuilder& outString, UErrorCode& status) const { if (U_FAILURE(status)) { return 0; } - fMicroPropsGenerator->processQuantity(inValue, fMicros, status); + // #13453: DecimalFormat wants the affixes from the pattern only (modMiddle, aka pattern modifier). + // Safe path: use fImmutablePatternModifier. + const Modifier* modifier = fImmutablePatternModifier->getModifier(signum, plural); + modifier->apply(outString, 0, 0, status); + if (U_FAILURE(status)) { return 0; } + return modifier->getPrefixLength(status); +} + +int32_t NumberFormatterImpl::getPrefixSuffixUnsafe(int8_t signum, StandardPlural::Form plural, + NumberStringBuilder& outString, UErrorCode& status) { if (U_FAILURE(status)) { return 0; } - // #13453: DecimalFormat wants the affixes from the pattern only (modMiddle). - fMicros.modMiddle->apply(outString, 0, 0, status); + // #13453: DecimalFormat wants the affixes from the pattern only (modMiddle, aka pattern modifier). + // Unsafe path: use fPatternModifier. + fPatternModifier->setNumberProperties(signum, plural); + fPatternModifier->apply(outString, 0, 0, status); if (U_FAILURE(status)) { return 0; } - return fMicros.modMiddle->getPrefixLength(status); + return fPatternModifier->getPrefixLength(status); } NumberFormatterImpl::NumberFormatterImpl(const MacroProps& macros, bool safe, UErrorCode& status) { diff --git a/icu4c/source/i18n/number_formatimpl.h b/icu4c/source/i18n/number_formatimpl.h index ef8ee7064ed..4e6c9ca50fc 100644 --- a/icu4c/source/i18n/number_formatimpl.h +++ b/icu4c/source/i18n/number_formatimpl.h @@ -43,13 +43,20 @@ class NumberFormatterImpl : public UMemory { * @return The index into the output at which the prefix ends and the suffix starts; in other words, * the prefix length. */ - static int32_t getPrefixSuffix(const MacroProps& macros, DecimalQuantity& inValue, - NumberStringBuilder& outString, UErrorCode& status); + static int32_t getPrefixSuffixStatic(const MacroProps& macros, int8_t signum, + StandardPlural::Form plural, NumberStringBuilder& outString, + UErrorCode& status); /** * Evaluates the "safe" MicroPropsGenerator created by "fromMacros". */ - void apply(DecimalQuantity &inValue, NumberStringBuilder &outString, UErrorCode &status) const; + void apply(DecimalQuantity& inValue, NumberStringBuilder& outString, UErrorCode& status) const; + + /** + * Like getPrefixSuffixStatic() but uses the safe compiled object. + */ + int32_t getPrefixSuffix(int8_t signum, StandardPlural::Form plural, NumberStringBuilder& outString, + UErrorCode& status) const; private: // Head of the MicroPropsGenerator linked list: @@ -64,7 +71,7 @@ class NumberFormatterImpl : public UMemory { LocalPointer fRules; LocalPointer fPatternInfo; LocalPointer fScientificHandler; - LocalPointer fPatternModifier; + LocalPointer fPatternModifier; LocalPointer fImmutablePatternModifier; LocalPointer fLongNameHandler; LocalPointer fCompactHandler; @@ -79,8 +86,8 @@ class NumberFormatterImpl : public UMemory { void applyUnsafe(DecimalQuantity &inValue, NumberStringBuilder &outString, UErrorCode &status); - int32_t getPrefixSuffixUnsafe(DecimalQuantity& inValue, NumberStringBuilder& outString, - UErrorCode& status); + int32_t getPrefixSuffixUnsafe(int8_t signum, StandardPlural::Form plural, + NumberStringBuilder& outString, UErrorCode& status); /** * If rulesPtr is non-null, return it. Otherwise, return a PluralRules owned by this object for the diff --git a/icu4c/source/i18n/number_patternmodifier.cpp b/icu4c/source/i18n/number_patternmodifier.cpp index 5b6c6bdb04f..52f0e4ef46d 100644 --- a/icu4c/source/i18n/number_patternmodifier.cpp +++ b/icu4c/source/i18n/number_patternmodifier.cpp @@ -137,6 +137,15 @@ void ImmutablePatternModifier::applyToMicros(MicroProps& micros, DecimalQuantity } } +const Modifier* ImmutablePatternModifier::getModifier(int8_t signum, StandardPlural::Form plural) const { + if (rules == nullptr) { + return pm->getModifier(signum); + } else { + return pm->getModifier(signum, plural); + } +} + + /** Used by the unsafe code path. */ MicroPropsGenerator& MutablePatternModifier::addToChain(const MicroPropsGenerator* parent) { this->parent = parent; diff --git a/icu4c/source/i18n/number_patternmodifier.h b/icu4c/source/i18n/number_patternmodifier.h index 74b6f8b83b5..a8c1f22f0b2 100644 --- a/icu4c/source/i18n/number_patternmodifier.h +++ b/icu4c/source/i18n/number_patternmodifier.h @@ -42,6 +42,8 @@ class U_I18N_API ImmutablePatternModifier : public MicroPropsGenerator, public U void applyToMicros(MicroProps& micros, DecimalQuantity& quantity) const; + const Modifier* getModifier(int8_t signum, StandardPlural::Form plural) const; + private: ImmutablePatternModifier(ParameterizedModifier* pm, const PluralRules* rules, const MicroPropsGenerator* parent); diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 5f4a6bb7ac5..b3f80afe6d2 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -2206,7 +2206,7 @@ class U_I18N_API LocalizedNumberFormatter /** Internal method for DecimalFormat compatibility. * @internal */ - void getAffix(bool isPrefix, bool isNegative, UnicodeString& result, UErrorCode& status) const; + void getAffixImpl(bool isPrefix, bool isNegative, UnicodeString& result, UErrorCode& status) const; /** * Internal method for testing. @@ -2293,6 +2293,11 @@ class U_I18N_API LocalizedNumberFormatter LocalizedNumberFormatter(impl::MacroProps &¯os, const Locale &locale); + /** + * @return true if the compiled formatter is available. + */ + bool computeCompiled(UErrorCode& status) const; + // To give the fluent setters access to this class's constructor: friend class NumberFormatterSettings; friend class NumberFormatterSettings; 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 609259ab078..801af947f0e 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 @@ -244,6 +244,17 @@ public class MutablePatternModifier implements Modifier, SymbolProvider, MicroPr micros.modMiddle = pm.getModifier(quantity.signum(), plural); } } + + // NOTE: This method is not used in ICU4J right now. + // In ICU4C, it is used by getPrefixSuffix(). + // Un-comment this method when getPrefixSuffix() is cleaned up in ICU4J. + // public Modifier getModifier(byte signum, StandardPlural plural) { + // if (rules == null) { + // return pm.getModifier(signum); + // } else { + // return pm.getModifier(signum, plural); + // } + // } } /** Used by the unsafe code path. */ diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/LocalizedNumberFormatter.java b/icu4j/main/classes/core/src/com/ibm/icu/number/LocalizedNumberFormatter.java index 9e1404c961f..245447f6da6 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/LocalizedNumberFormatter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/LocalizedNumberFormatter.java @@ -5,6 +5,7 @@ package com.ibm.icu.number; import java.math.BigInteger; import java.util.concurrent.atomic.AtomicLongFieldUpdater; +import com.ibm.icu.impl.StandardPlural; import com.ibm.icu.impl.Utility; import com.ibm.icu.impl.number.DecimalQuantity; import com.ibm.icu.impl.number.DecimalQuantity_DualStorageBCD; @@ -130,19 +131,11 @@ public class LocalizedNumberFormatter extends NumberFormatterSettings