From d02b30fc3f665677a775950b550179a8b3b736b2 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 22 Nov 2022 17:00:51 -0800 Subject: [PATCH] ICU-20115 ICU4C: Move SimpleDateFormat over to SimpleNumberFormatter --- icu4c/source/i18n/smpdtfmt.cpp | 162 ++++++++---------------- icu4c/source/i18n/unicode/smpdtfmt.h | 35 ++--- icu4c/source/test/intltest/dtfmttst.cpp | 7 + 3 files changed, 72 insertions(+), 132 deletions(-) diff --git a/icu4c/source/i18n/smpdtfmt.cpp b/icu4c/source/i18n/smpdtfmt.cpp index 72ba0c0bfc7..eef3d7766bb 100644 --- a/icu4c/source/i18n/smpdtfmt.cpp +++ b/icu4c/source/i18n/smpdtfmt.cpp @@ -45,6 +45,7 @@ #include "unicode/ustring.h" #include "unicode/basictz.h" #include "unicode/simpleformatter.h" +#include "unicode/simplenumberformatter.h" #include "unicode/simpletz.h" #include "unicode/rbtz.h" #include "unicode/tzfmt.h" @@ -334,7 +335,7 @@ SimpleDateFormat::~SimpleDateFormat() if (fTimeZoneFormat) { delete fTimeZoneFormat; } - freeFastNumberFormatters(); + delete fSimpleNumberFormatter; #if !UCONFIG_NO_BREAK_ITERATION delete fCapitalizationBrkIter; @@ -344,11 +345,7 @@ SimpleDateFormat::~SimpleDateFormat() //---------------------------------------------------------------------- SimpleDateFormat::SimpleDateFormat(UErrorCode& status) - : fLocale(Locale::getDefault()), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + : fLocale(Locale::getDefault()) { initializeBooleanAttributes(); construct(kShort, (EStyle) (kShort + kDateOffset), fLocale, status); @@ -360,11 +357,7 @@ SimpleDateFormat::SimpleDateFormat(UErrorCode& status) SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, UErrorCode &status) : fPattern(pattern), - fLocale(Locale::getDefault()), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(Locale::getDefault()) { fDateOverride.setToBogus(); fTimeOverride.setToBogus(); @@ -381,11 +374,7 @@ SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, const UnicodeString& override, UErrorCode &status) : fPattern(pattern), - fLocale(Locale::getDefault()), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(Locale::getDefault()) { fDateOverride.setTo(override); fTimeOverride.setToBogus(); @@ -405,10 +394,7 @@ SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, const Locale& locale, UErrorCode& status) : fPattern(pattern), - fLocale(locale), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(locale) { fDateOverride.setToBogus(); @@ -428,10 +414,7 @@ SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, const Locale& locale, UErrorCode& status) : fPattern(pattern), - fLocale(locale), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(locale) { fDateOverride.setTo(override); @@ -454,10 +437,7 @@ SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, UErrorCode& status) : fPattern(pattern), fLocale(Locale::getDefault()), - fSymbols(symbolsToAdopt), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fSymbols(symbolsToAdopt) { fDateOverride.setToBogus(); @@ -476,10 +456,7 @@ SimpleDateFormat::SimpleDateFormat(const UnicodeString& pattern, UErrorCode& status) : fPattern(pattern), fLocale(Locale::getDefault()), - fSymbols(new DateFormatSymbols(symbols)), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fSymbols(new DateFormatSymbols(symbols)) { fDateOverride.setToBogus(); @@ -498,11 +475,7 @@ SimpleDateFormat::SimpleDateFormat(EStyle timeStyle, EStyle dateStyle, const Locale& locale, UErrorCode& status) -: fLocale(locale), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) +: fLocale(locale) { initializeBooleanAttributes(); construct(timeStyle, dateStyle, fLocale, status); @@ -521,11 +494,7 @@ SimpleDateFormat::SimpleDateFormat(EStyle timeStyle, SimpleDateFormat::SimpleDateFormat(const Locale& locale, UErrorCode& status) : fPattern(gDefaultPattern), - fLocale(locale), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(locale) { if (U_FAILURE(status)) return; initializeBooleanAttributes(); @@ -557,11 +526,7 @@ SimpleDateFormat::SimpleDateFormat(const Locale& locale, SimpleDateFormat::SimpleDateFormat(const SimpleDateFormat& other) : DateFormat(other), - fLocale(other.fLocale), - fSymbols(NULL), - fTimeZoneFormat(NULL), - fSharedNumberFormatters(NULL), - fCapitalizationBrkIter(NULL) + fLocale(other.fLocale) { initializeBooleanAttributes(); *this = other; @@ -574,6 +539,12 @@ SimpleDateFormat& SimpleDateFormat::operator=(const SimpleDateFormat& other) if (this == &other) { return *this; } + + // fSimpleNumberFormatter references fNumberFormatter, delete it + // before we call the = operator which may invalidate fNumberFormatter + delete fSimpleNumberFormatter; + fSimpleNumberFormatter = nullptr; + DateFormat::operator=(other); fDateOverride = other.fDateOverride; fTimeOverride = other.fTimeOverride; @@ -642,9 +613,10 @@ SimpleDateFormat& SimpleDateFormat::operator=(const SimpleDateFormat& other) } UErrorCode localStatus = U_ZERO_ERROR; - freeFastNumberFormatters(); - initFastNumberFormatters(localStatus); - + // SimpleNumberFormatter does not have a copy constructor. Furthermore, + // it references data from an internal field, fNumberFormatter, + // so we must rematerialize that reference after copying over the number formatter. + initSimpleNumberFormatter(localStatus); return *this; } @@ -979,7 +951,7 @@ SimpleDateFormat::initialize(const Locale& locale, //fNumberFormat->setLenient(true); // Java uses a custom DateNumberFormat to format/parse initNumberFormatters(locale, status); - initFastNumberFormatters(status); + initSimpleNumberFormatter(status); } else if (U_SUCCESS(status)) @@ -1324,18 +1296,8 @@ _appendSymbolWithMonthPattern(UnicodeString& dst, int32_t value, const UnicodeSt //---------------------------------------------------------------------- -static number::LocalizedNumberFormatter* -createFastFormatter(const DecimalFormat* df, int32_t minInt, int32_t maxInt, UErrorCode& status) { - const number::LocalizedNumberFormatter* lnfBase = df->toNumberFormatter(status); - if (U_FAILURE(status)) { - return nullptr; - } - return lnfBase->integerWidth( - number::IntegerWidth::zeroFillTo(minInt).truncateAt(maxInt) - ).clone().orphan(); -} - -void SimpleDateFormat::initFastNumberFormatters(UErrorCode& status) { +void +SimpleDateFormat::initSimpleNumberFormatter(UErrorCode &status) { if (U_FAILURE(status)) { return; } @@ -1343,27 +1305,20 @@ void SimpleDateFormat::initFastNumberFormatters(UErrorCode& status) { if (df == nullptr) { return; } - fFastNumberFormatters[SMPDTFMT_NF_1x10] = createFastFormatter(df, 1, 10, status); - fFastNumberFormatters[SMPDTFMT_NF_2x10] = createFastFormatter(df, 2, 10, status); - fFastNumberFormatters[SMPDTFMT_NF_3x10] = createFastFormatter(df, 3, 10, status); - fFastNumberFormatters[SMPDTFMT_NF_4x10] = createFastFormatter(df, 4, 10, status); - fFastNumberFormatters[SMPDTFMT_NF_2x2] = createFastFormatter(df, 2, 2, status); -} - -void SimpleDateFormat::freeFastNumberFormatters() { - delete fFastNumberFormatters[SMPDTFMT_NF_1x10]; - delete fFastNumberFormatters[SMPDTFMT_NF_2x10]; - delete fFastNumberFormatters[SMPDTFMT_NF_3x10]; - delete fFastNumberFormatters[SMPDTFMT_NF_4x10]; - delete fFastNumberFormatters[SMPDTFMT_NF_2x2]; - fFastNumberFormatters[SMPDTFMT_NF_1x10] = nullptr; - fFastNumberFormatters[SMPDTFMT_NF_2x10] = nullptr; - fFastNumberFormatters[SMPDTFMT_NF_3x10] = nullptr; - fFastNumberFormatters[SMPDTFMT_NF_4x10] = nullptr; - fFastNumberFormatters[SMPDTFMT_NF_2x2] = nullptr; + const DecimalFormatSymbols* syms = df->getDecimalFormatSymbols(); + if (syms == nullptr) { + return; + } + fSimpleNumberFormatter = new number::SimpleNumberFormatter( + number::SimpleNumberFormatter::forLocaleAndSymbolsAndGroupingStrategy( + fLocale, *syms, UNUM_GROUPING_OFF, status + ) + ); + if (fSimpleNumberFormatter == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + } } - void SimpleDateFormat::initNumberFormatters(const Locale &locale,UErrorCode &status) { if (U_FAILURE(status)) { @@ -2116,6 +2071,11 @@ SimpleDateFormat::subFormat(UnicodeString &appendTo, //---------------------------------------------------------------------- void SimpleDateFormat::adoptNumberFormat(NumberFormat *formatToAdopt) { + // Null out the fast formatter, it references fNumberFormat which we're + // about to invalidate + delete fSimpleNumberFormatter; + fSimpleNumberFormatter = nullptr; + fixNumberFormatForDates(*formatToAdopt); delete fNumberFormat; fNumberFormat = formatToAdopt; @@ -2127,10 +2087,9 @@ void SimpleDateFormat::adoptNumberFormat(NumberFormat *formatToAdopt) { fSharedNumberFormatters = NULL; } - // Also re-compute the fast formatters. + // Recompute fSimpleNumberFormatter if necessary UErrorCode localStatus = U_ZERO_ERROR; - freeFastNumberFormatters(); - initFastNumberFormatters(localStatus); + initSimpleNumberFormatter(localStatus); } void SimpleDateFormat::adoptNumberFormat(const UnicodeString& fields, NumberFormat *formatToAdopt, UErrorCode &status){ @@ -2186,36 +2145,19 @@ SimpleDateFormat::zeroPaddingNumber( UnicodeString &appendTo, int32_t value, int32_t minDigits, int32_t maxDigits) const { - const number::LocalizedNumberFormatter* fastFormatter = nullptr; - // NOTE: This uses the heuristic that these five min/max int settings account for the vast majority - // of SimpleDateFormat number formatting cases at the time of writing (ICU 62). - if (currentNumberFormat == fNumberFormat) { - if (maxDigits == 10) { - if (minDigits == 1) { - fastFormatter = fFastNumberFormatters[SMPDTFMT_NF_1x10]; - } else if (minDigits == 2) { - fastFormatter = fFastNumberFormatters[SMPDTFMT_NF_2x10]; - } else if (minDigits == 3) { - fastFormatter = fFastNumberFormatters[SMPDTFMT_NF_3x10]; - } else if (minDigits == 4) { - fastFormatter = fFastNumberFormatters[SMPDTFMT_NF_4x10]; - } - } else if (maxDigits == 2) { - if (minDigits == 2) { - fastFormatter = fFastNumberFormatters[SMPDTFMT_NF_2x2]; - } - } - } - if (fastFormatter != nullptr) { + + if (currentNumberFormat == fNumberFormat && fSimpleNumberFormatter) { // Can use fast path - number::impl::UFormattedNumberData result; - result.quantity.setToInt(value); UErrorCode localStatus = U_ZERO_ERROR; - fastFormatter->formatImpl(&result, localStatus); + number::SimpleNumber number = number::SimpleNumber::forInt64(value, localStatus); + number.setMinimumIntegerDigits(minDigits, localStatus); + number.truncateStart(maxDigits, localStatus); + + number::FormattedNumber result = fSimpleNumberFormatter->format(std::move(number), localStatus); if (U_FAILURE(localStatus)) { return; } - appendTo.append(result.getStringRef().toTempUnicodeString()); + appendTo.append(result.toTempString(localStatus)); return; } diff --git a/icu4c/source/i18n/unicode/smpdtfmt.h b/icu4c/source/i18n/unicode/smpdtfmt.h index fb83f90da1e..6143bfcb9db 100644 --- a/icu4c/source/i18n/unicode/smpdtfmt.h +++ b/icu4c/source/i18n/unicode/smpdtfmt.h @@ -55,6 +55,7 @@ class DateIntervalFormat; namespace number { class LocalizedNumberFormatter; +class SimpleNumberFormatter; } /** @@ -1520,14 +1521,9 @@ private: int32_t skipUWhiteSpace(const UnicodeString& text, int32_t pos) const; /** - * Initialize LocalizedNumberFormatter instances used for speedup. + * Initialize SimpleNumberFormat instance */ - void initFastNumberFormatters(UErrorCode& status); - - /** - * Delete the LocalizedNumberFormatter instances used for speedup. - */ - void freeFastNumberFormatters(); + void initSimpleNumberFormatter(UErrorCode &status); /** * Initialize NumberFormat instances used for numbering system overrides. @@ -1599,12 +1595,12 @@ private: * A pointer to an object containing the strings to use in formatting (e.g., * month and day names, AM and PM strings, time zone names, etc.) */ - DateFormatSymbols* fSymbols; // Owned + DateFormatSymbols* fSymbols = nullptr; // Owned /** * The time zone formatter */ - TimeZoneFormat* fTimeZoneFormat; + TimeZoneFormat* fTimeZoneFormat = nullptr; /** * If dates have ambiguous years, we map them into the century starting @@ -1644,25 +1640,20 @@ private: * The number format in use for each date field. NULL means fall back * to fNumberFormat in DateFormat. */ - const SharedNumberFormat **fSharedNumberFormatters; - - enum NumberFormatterKey { - SMPDTFMT_NF_1x10, - SMPDTFMT_NF_2x10, - SMPDTFMT_NF_3x10, - SMPDTFMT_NF_4x10, - SMPDTFMT_NF_2x2, - SMPDTFMT_NF_COUNT - }; + const SharedNumberFormat **fSharedNumberFormatters = nullptr; /** - * Number formatters pre-allocated for fast performance on the most common integer lengths. + * Number formatter pre-allocated for fast performance + * + * This references the decimal symbols from fNumberFormatter if it is an instance + * of DecimalFormat (and is otherwise null). This should always be cleaned up before + * destroying fNumberFormatter. */ - const number::LocalizedNumberFormatter* fFastNumberFormatters[SMPDTFMT_NF_COUNT] = {}; + const number::SimpleNumberFormatter* fSimpleNumberFormatter = nullptr; UBool fHaveDefaultCentury; - const BreakIterator* fCapitalizationBrkIter; + const BreakIterator* fCapitalizationBrkIter = nullptr; }; inline UDate diff --git a/icu4c/source/test/intltest/dtfmttst.cpp b/icu4c/source/test/intltest/dtfmttst.cpp index e29c7a16f36..c51cad37c1a 100644 --- a/icu4c/source/test/intltest/dtfmttst.cpp +++ b/icu4c/source/test/intltest/dtfmttst.cpp @@ -4869,6 +4869,13 @@ void DateFormatTest::TestNumberFormatOverride() { if (result != expected) errln("FAIL: Expected " + expected + " get: " + result); + + // Ensure that adopted formats are handled correctly after copy constructing + SimpleDateFormat fmtCopy = *fmt; + result.remove(); + fmtCopy.format(test_date,result, pos); + if (result != expected) + errln("FAIL: Expected " + expected + " after copy constructing get: " + result); } } -- 2.40.0