From 597e3287fc17e3551d7acf58462e3fa647f21a9e Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Tue, 17 Apr 2018 10:18:42 +0000 Subject: [PATCH] ICU-13135 Fixing and optimizing PluralFormat call site into NumberFormat in order to execute the formatting pipeline only once. X-SVN-Rev: 41237 --- icu4c/source/i18n/fmtable.cpp | 51 +++++++++++-------- icu4c/source/i18n/plurfmt.cpp | 41 +++++++-------- icu4c/source/i18n/unicode/fmtable.h | 6 +++ icu4c/source/test/intltest/plurfmts.cpp | 11 ++-- .../DecimalQuantity_DualStorageBCD.java | 2 + .../src/com/ibm/icu/text/NumberFormat.java | 2 + .../src/com/ibm/icu/text/PluralFormat.java | 25 ++++++--- 7 files changed, 86 insertions(+), 52 deletions(-) diff --git a/icu4c/source/i18n/fmtable.cpp b/icu4c/source/i18n/fmtable.cpp index dd465a60e0f..f1b98a36441 100644 --- a/icu4c/source/i18n/fmtable.cpp +++ b/icu4c/source/i18n/fmtable.cpp @@ -716,28 +716,11 @@ CharString *Formattable::internalGetCharString(UErrorCode &status) { // from parsing, or from the user setting a decimal number, fDecimalNum // would already be set. // - fDecimalQuantity = new DecimalQuantity(); // TODO: use internal digit list - if (fDecimalQuantity == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - return NULL; - } - - switch (fType) { - case kDouble: - fDecimalQuantity->setToDouble(this->getDouble()); - fDecimalQuantity->roundToInfinity(); - break; - case kLong: - fDecimalQuantity->setToInt(this->getLong()); - break; - case kInt64: - fDecimalQuantity->setToLong(this->getInt64()); - break; - default: - // The formattable's value is not a numeric type. - status = U_INVALID_STATE_ERROR; - return NULL; - } + LocalPointer dq(new DecimalQuantity(), status); + if (U_FAILURE(status)) { return nullptr; } + populateDecimalQuantity(*dq, status); + if (U_FAILURE(status)) { return nullptr; } + fDecimalQuantity = dq.orphan(); } fDecimalStr = new CharString(); @@ -759,6 +742,30 @@ CharString *Formattable::internalGetCharString(UErrorCode &status) { return fDecimalStr; } +void +Formattable::populateDecimalQuantity(number::impl::DecimalQuantity& output, UErrorCode& status) const { + if (fDecimalQuantity != nullptr) { + output = *fDecimalQuantity; + return; + } + + switch (fType) { + case kDouble: + output.setToDouble(this->getDouble()); + output.roundToInfinity(); + break; + case kLong: + output.setToInt(this->getLong()); + break; + case kInt64: + output.setToLong(this->getInt64()); + break; + default: + // The formattable's value is not a numeric type. + status = U_INVALID_STATE_ERROR; + } +} + // --------------------------------------- void Formattable::adoptDecimalQuantity(DecimalQuantity *dq) { diff --git a/icu4c/source/i18n/plurfmt.cpp b/icu4c/source/i18n/plurfmt.cpp index eb8b5d88a8f..2775766d32d 100644 --- a/icu4c/source/i18n/plurfmt.cpp +++ b/icu4c/source/i18n/plurfmt.cpp @@ -22,6 +22,8 @@ #include "uassert.h" #include "uhash.h" #include "number_decimalquantity.h" +#include "number_utils.h" +#include "number_utypes.h" #if !UCONFIG_NO_FORMATTING @@ -259,34 +261,33 @@ PluralFormat::format(const Formattable& numberObject, double number, if (msgPattern.countParts() == 0) { return numberFormat->format(numberObject, appendTo, pos, status); } + // Get the appropriate sub-message. // Select it based on the formatted number-offset. double numberMinusOffset = number - offset; - UnicodeString numberString; - DecimalQuantity quantity; - FieldPosition ignorePos; + // Call NumberFormatter to get both the DecimalQuantity and the string. + // This call site needs to use more internal APIs than the Java equivalent. + number::impl::UFormattedNumberData data; if (offset == 0) { - DecimalFormat *decFmt = dynamic_cast(numberFormat); - if(decFmt != NULL) { - decFmt->formatToDecimalQuantity(numberObject, quantity, status); - } else { - numberFormat->format( - numberObject, numberString, ignorePos, status); // could be BigDecimal etc. - quantity.setToDouble(numberMinusOffset); - quantity.roundToInfinity(); - } + // could be BigDecimal etc. + numberObject.populateDecimalQuantity(data.quantity, status); } else { - DecimalFormat *decFmt = dynamic_cast(numberFormat); - if(decFmt != NULL) { - decFmt->formatToDecimalQuantity(numberMinusOffset, quantity, status); + data.quantity.setToDouble(numberMinusOffset); + } + UnicodeString numberString; + auto *decFmt = dynamic_cast(numberFormat); + if(decFmt != nullptr) { + decFmt->toNumberFormatter().formatImpl(&data, status); // mutates &data + numberString = data.string.toUnicodeString(); + } else { + if (offset == 0) { + numberFormat->format(numberObject, numberString, status); } else { - numberFormat->format( - numberMinusOffset, numberString, ignorePos, status); - quantity.setToDouble(numberMinusOffset); - quantity.roundToInfinity(); + numberFormat->format(numberMinusOffset, numberString, status); } } - int32_t partIndex = findSubMessage(msgPattern, 0, pluralRulesWrapper, &quantity, number, status); + + int32_t partIndex = findSubMessage(msgPattern, 0, pluralRulesWrapper, &data.quantity, number, status); if (U_FAILURE(status)) { return appendTo; } // Replace syntactic # signs in the top level of this sub-message // (not in nested arguments) with the formatted number-offset. diff --git a/icu4c/source/i18n/unicode/fmtable.h b/icu4c/source/i18n/unicode/fmtable.h index 9f729c918a4..e27cb9e8087 100644 --- a/icu4c/source/i18n/unicode/fmtable.h +++ b/icu4c/source/i18n/unicode/fmtable.h @@ -659,6 +659,12 @@ public: */ number::impl::DecimalQuantity *getDecimalQuantity() const { return fDecimalQuantity;} + /** + * Export the value of this Formattable to a DecimalQuantity. + * @internal + */ + void populateDecimalQuantity(number::impl::DecimalQuantity& output, UErrorCode& status) const; + /** * Adopt, and set value from, a DecimalQuantity * Internal Function, do not use. diff --git a/icu4c/source/test/intltest/plurfmts.cpp b/icu4c/source/test/intltest/plurfmts.cpp index db2485d31f7..c9f49f74c8a 100644 --- a/icu4c/source/test/intltest/plurfmts.cpp +++ b/icu4c/source/test/intltest/plurfmts.cpp @@ -55,7 +55,7 @@ void PluralFormatTest::pluralFormatBasicTest(/*char *par*/) PluralFormat* plFmt[8]; Locale locale = Locale::getDefault(); UnicodeString otherPattern = UnicodeString("other{#}"); - UnicodeString message=UnicodeString("ERROR: PluralFormat basic test"); + UnicodeString message=UnicodeString("PluralFormat basic test"); // ========= Test constructors logln(" Testing PluralFormat constructors ..."); @@ -685,7 +685,7 @@ PluralFormatTest::TestDecimals() { } void -PluralFormatTest::numberFormatTest(PluralFormat* plFmt, +PluralFormatTest::numberFormatTest(PluralFormat* plFmt, NumberFormat *numFmt, int32_t start, int32_t end, @@ -723,12 +723,15 @@ PluralFormatTest::numberFormatTest(PluralFormat* plFmt, } } } - if ( (numResult!=plResult) || U_FAILURE(status) ) { + if (U_FAILURE(status)) { + assertSuccess(*message + " in numberFormatTest", status); + } + if (numResult!=plResult) { if ( message == NULL ) { errln("ERROR: Unexpected plural format - got:"+plResult+ UnicodeString(" expecting:")+numResult); } else { - errln( *message+UnicodeString(" got:")+plResult+UnicodeString(" expecting:")+numResult); + assertEquals(*message + " in numberFormatTest", numResult, plResult); } } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_DualStorageBCD.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_DualStorageBCD.java index 8d8e791ccaa..f298d3f6053 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_DualStorageBCD.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_DualStorageBCD.java @@ -60,6 +60,8 @@ public final class DecimalQuantity_DualStorageBCD extends DecimalQuantity_Abstra } public DecimalQuantity_DualStorageBCD(Number number) { + // NOTE: Number type expansion happens both here + // and in NumberFormat.java if (number instanceof Long) { setToLong(number.longValue()); } else if (number instanceof Integer) { diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/NumberFormat.java b/icu4j/main/classes/core/src/com/ibm/icu/text/NumberFormat.java index 63d2a109c40..36779197b0f 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/text/NumberFormat.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/text/NumberFormat.java @@ -269,6 +269,8 @@ public abstract class NumberFormat extends UFormat { public StringBuffer format(Object number, StringBuffer toAppendTo, FieldPosition pos) { + // NOTE: Number type expansion happens both here + // and in DecimalQuantity_DualStorageBCD.java if (number instanceof Long) { return format(((Long)number).longValue(), toAppendTo, pos); } else if (number instanceof BigInteger) { diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/PluralFormat.java b/icu4j/main/classes/core/src/com/ibm/icu/text/PluralFormat.java index 1918283dc5c..1d9afb97d70 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/text/PluralFormat.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/text/PluralFormat.java @@ -17,6 +17,8 @@ import java.util.Locale; import java.util.Map; import com.ibm.icu.impl.Utility; +import com.ibm.icu.number.FormattedNumber; +import com.ibm.icu.number.LocalizedNumberFormatter; import com.ibm.icu.text.PluralRules.FixedDecimal; import com.ibm.icu.text.PluralRules.IFixedDecimal; import com.ibm.icu.text.PluralRules.PluralType; @@ -613,17 +615,28 @@ public class PluralFormat extends UFormat { // Select it based on the formatted number-offset. double numberMinusOffset = number - offset; String numberString; - if (offset == 0) { - numberString = numberFormat.format(numberObject); // could be BigDecimal etc. - } else { - numberString = numberFormat.format(numberMinusOffset); - } IFixedDecimal dec; if(numberFormat instanceof DecimalFormat) { - dec = ((DecimalFormat) numberFormat).getFixedDecimal(numberMinusOffset); + // Call NumberFormatter to get both the DecimalQuantity and the string. + LocalizedNumberFormatter f = ((DecimalFormat) numberFormat).toNumberFormatter(); + FormattedNumber result; + if (offset == 0) { + // could be BigDecimal etc. + result = f.format(numberObject); + } else { + result = f.format(numberMinusOffset); + } + numberString = result.toString(); + dec = result.getFixedDecimal(); } else { + if (offset == 0) { + numberString = numberFormat.format(numberObject); + } else { + numberString = numberFormat.format(numberMinusOffset); + } dec = new FixedDecimal(numberMinusOffset); } + int partIndex = findSubMessage(msgPattern, 0, pluralRulesWrapper, dec, number); // Replace syntactic # signs in the top level of this sub-message // (not in nested arguments) with the formatted number-offset. -- 2.40.0