From 9c99964ddf4842f38af1a98609614cc7f00c3b44 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 21 Jul 2020 02:37:06 +0200 Subject: [PATCH 01/25] Implement Precision handling in UsagePrefsHandler::processQuantity --- icu4c/source/i18n/number_integerwidth.cpp | 3 ++ icu4c/source/i18n/number_longnames.cpp | 3 ++ icu4c/source/i18n/number_rounding.cpp | 3 ++ icu4c/source/i18n/number_roundingutils.h | 3 ++ icu4c/source/i18n/number_skeletons.cpp | 11 +++- icu4c/source/i18n/number_usageprefs.cpp | 53 ++++++++++++++----- icu4c/source/i18n/unitsrouter.h | 9 ++++ icu4c/source/test/intltest/numbertest_api.cpp | 39 ++++++++++---- 8 files changed, 98 insertions(+), 26 deletions(-) diff --git a/icu4c/source/i18n/number_integerwidth.cpp b/icu4c/source/i18n/number_integerwidth.cpp index d62aef444dc..10b853423c8 100644 --- a/icu4c/source/i18n/number_integerwidth.cpp +++ b/icu4c/source/i18n/number_integerwidth.cpp @@ -40,6 +40,9 @@ IntegerWidth IntegerWidth::truncateAt(int32_t maxInt) { } void IntegerWidth::apply(impl::DecimalQuantity& quantity, UErrorCode& status) const { + if (U_FAILURE(status)) { + return; + } if (fHasError) { status = U_ILLEGAL_ARGUMENT_ERROR; } else if (fUnion.minMaxInt.fMaxInt == -1) { diff --git a/icu4c/source/i18n/number_longnames.cpp b/icu4c/source/i18n/number_longnames.cpp index 9d58895c6dd..32a5be9d591 100644 --- a/icu4c/source/i18n/number_longnames.cpp +++ b/icu4c/source/i18n/number_longnames.cpp @@ -426,6 +426,9 @@ void LongNameMultiplexer::processQuantity(DecimalQuantity &quantity, MicroProps return; } } + if (U_FAILURE(status)) { + return; + } // We shouldn't receive any outputUnit for which we haven't already got a // LongNameHandler: status = U_INTERNAL_PROGRAM_ERROR; diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index 3ffce673ad0..ee0a6985f1b 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -341,6 +341,9 @@ RoundingImpl::chooseMultiplierAndApply(impl::DecimalQuantity &input, const impl: /** This is the method that contains the actual rounding logic. */ void RoundingImpl::apply(impl::DecimalQuantity &value, UErrorCode& status) const { + if (U_FAILURE(status)) { + return; + } if (fPassThrough) { return; } diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index 3e37f319540..f672641cf61 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -44,6 +44,9 @@ enum Section { inline bool getRoundingDirection(bool isEven, bool isNegative, Section section, RoundingMode roundingMode, UErrorCode &status) { + if (U_FAILURE(status)) { + return false; + } switch (roundingMode) { case RoundingMode::UNUM_ROUND_UP: // round away from zero diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index 46c2125f1d2..eeefadfe3cc 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -484,6 +484,15 @@ MacroProps skeleton::parseSkeleton( const UnicodeString& skeletonString, int32_t& errOffset, UErrorCode& status) { U_ASSERT(U_SUCCESS(status)); + // FIXME (DO NOT SUBMIT): In this PR, we consider calling parseSkeleton + // directly rather than going through create(). With the first call, we've + // missed initNumberSkeletons though. Do we want to go through create, which + // is just a little bit wasteful, or do we want to find an alternative way + // to ensure initNumberSkeletons has been called? + umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); + + U_ASSERT(kSerializedStemTrie != nullptr); + // Add a trailing whitespace to the end of the skeleton string to make code cleaner. UnicodeString tempSkeletonString(skeletonString); tempSkeletonString.append(u' '); @@ -1583,7 +1592,7 @@ bool GeneratorHelpers::perUnit(const MacroProps& macros, UnicodeString& sb, UErr } } -bool GeneratorHelpers::usage(const MacroProps& macros, UnicodeString& sb, UErrorCode&) { +bool GeneratorHelpers::usage(const MacroProps& macros, UnicodeString& sb, UErrorCode& /* status */) { if (macros.usage.fLength > 0) { sb.append(u"usage/", -1); sb.append(UnicodeString(macros.usage.fUsage, -1, US_INV)); diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 0c7988e32de..d239764b4b9 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -8,6 +8,7 @@ #include "number_decimalquantity.h" #include "number_microprops.h" #include "number_roundingutils.h" +#include "number_skeletons.h" #include "unicode/char16ptr.h" #include "unicode/currunit.h" #include "unicode/fmtable.h" @@ -19,6 +20,7 @@ using namespace icu::number; using namespace icu::number::impl; +using icu::number::impl::skeleton::parseSkeleton; // Copy constructor Usage::Usage(const Usage &other) : fUsage(nullptr), fLength(other.fLength), fError(other.fError) { @@ -100,24 +102,47 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m quantity.roundToInfinity(); // Enables toDouble const auto routed = fUnitsRouter.route(quantity.toDouble(), status); + if (U_FAILURE(status)) { + return; + } const auto& routedUnits = routed.measures; micros.outputUnit = routedUnits[0]->getUnit(); quantity.setToDouble(routedUnits[0]->getNumber().getDouble()); - // TODO(units): here we are always overriding Precision. (1) get precision - // from fUnitsRouter, (2) ensure we use the UnitPreference skeleton's - // precision only when there isn't an explicit override we prefer to use. - // This needs to be handled within - // NumberFormatterImpl::macrosToMicroGenerator in number_formatimpl.cpp - // TODO: Use precision from `routed` result. - Precision precision = Precision::integer().withMinDigits(2); - UNumberFormatRoundingMode roundingMode; - // Temporary until ICU 64? - roundingMode = precision.fRoundingMode; - CurrencyUnit currency(u"", status); - micros.rounder = {precision, roundingMode, currency, status}; - if (U_FAILURE(status)) { - return; + UnicodeString precisionSkeleton = routed.precision; + // TODO(icu-units/icu#13): If the programmer specified a precision, use + // that. + if (precisionSkeleton.length() > 0) { + CharString csPrecisionSkeleton; + UErrorCode csErrCode = U_ZERO_ERROR; + csPrecisionSkeleton.appendInvariantChars(precisionSkeleton, csErrCode); + + // Parse skeleton, collect results + int32_t errOffset; + // int32_t errOffset = 0; + U_ASSERT(U_SUCCESS(status)); + MacroProps unitPrefMacros = parseSkeleton(precisionSkeleton, errOffset, status); + Precision precision; + UNumberFormatRoundingMode roundingMode; + if (U_FAILURE(status)) { + return; + } + precision = unitPrefMacros.precision; + if (unitPrefMacros.roundingMode != kDefaultMode) { + roundingMode = unitPrefMacros.roundingMode; + } else { + // TODO: "Temporary until ICU 64" in number_formatimpl.cpp? Clarify! + roundingMode = precision.fRoundingMode; + } + CurrencyUnit currency(u"", status); + micros.rounder = {precision, roundingMode, currency, status}; + } else { + Precision precision = Precision::integer().withMinDigits(2); + UNumberFormatRoundingMode roundingMode; + // Temporary until ICU 64? + roundingMode = precision.fRoundingMode; + CurrencyUnit currency(u"", status); + micros.rounder = {precision, roundingMode, currency, status}; } } diff --git a/icu4c/source/i18n/unitsrouter.h b/icu4c/source/i18n/unitsrouter.h index 68284663293..b98ee83a786 100644 --- a/icu4c/source/i18n/unitsrouter.h +++ b/icu4c/source/i18n/unitsrouter.h @@ -25,7 +25,16 @@ class Measure; namespace units { struct RouteResult : UMemory { + // A list of measures: a single measure for single units, multiple measures + // for mixed units. + // + // TODO(icu-units/icu#21): figure out the right mixed unit API. MaybeStackVector measures; + + // A skeleton string starting with a precision-increment. + // + // TODO(hugovdm): generalise? or narrow down to only a precision-increment? + // or document that other skeleton elements are ignored? UnicodeString precision; RouteResult(MaybeStackVector measures, UnicodeString precision) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 4c50e33add0..a04c2772f53 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -684,8 +684,12 @@ void NumberFormatterApiTest::unitUsage() { IcuTestErrorCode status(*this, "unitUsage()"); - LocalizedNumberFormatter formatter = unloc_formatter.locale("en-ZA"); - FormattedNumber formattedNum = formatter.formatDouble(300, status); + LocalizedNumberFormatter formatter; + FormattedNumber formattedNum; + + formatter = unloc_formatter.locale("en-ZA"); + formattedNum = formatter.formatDouble(304, status); + status.errIfFailureAndReset("unitUsage() en-ZA road, formatDouble(...)"); assertTrue(UnicodeString("unitUsage() en-ZA road, got outputUnit: \"") + formattedNum.getOutputUnit(status).getIdentifier() + "\"", MeasureUnit::getMeter() == formattedNum.getOutputUnit(status)); @@ -701,17 +705,23 @@ void NumberFormatterApiTest::unitUsage() { u"877 km", u"88 km", u"8,8 km", - u"877 m", - u"88 m", - u"8,8 m", + u"900 m", + u"90 m", + u"10 m", u"0 m"); formatter = unloc_formatter.locale("en-GB"); - formattedNum = formatter.formatDouble(300, status); + formattedNum = formatter.formatDouble(304, status); + status.errIfFailureAndReset("unitUsage() en-GB road, formatDouble(...)"); + U_ASSERT(status == U_ZERO_ERROR); assertTrue(UnicodeString("unitUsage() en-GB road, got outputUnit: \"") + formattedNum.getOutputUnit(status).getIdentifier() + "\"", MeasureUnit::getYard() == formattedNum.getOutputUnit(status)); - assertEquals("unitUsage() en-GB road", "328 yd", formattedNum.toString(status)); + status.errIfFailureAndReset("unitUsage() en-GB road, getOutputUnit(...)"); + U_ASSERT(status == U_ZERO_ERROR); + assertEquals("unitUsage() en-GB road", "350 yd", formattedNum.toString(status)); + status.errIfFailureAndReset("unitUsage() en-GB road, toString(...)"); + U_ASSERT(status == U_ZERO_ERROR); assertFormatDescendingBig( u"unitUsage() en-GB road", u"measure-unit/length-meter usage/road", @@ -729,11 +739,17 @@ void NumberFormatterApiTest::unitUsage() { u"0 yd"); formatter = unloc_formatter.locale("en-US"); - formattedNum = formatter.formatDouble(300, status); + formattedNum = formatter.formatDouble(304, status); + status.errIfFailureAndReset("unitUsage() en-US road, formatDouble(...)"); + U_ASSERT(status == U_ZERO_ERROR); assertTrue(UnicodeString("unitUsage() en-US road, got outputUnit: \"") + formattedNum.getOutputUnit(status).getIdentifier() + "\"", MeasureUnit::getFoot() == formattedNum.getOutputUnit(status)); - assertEquals("unitUsage() en-US road", "984 ft", formattedNum.toString(status)); + status.errIfFailureAndReset("unitUsage() en-US road, getOutputUnit(...)"); + U_ASSERT(status == U_ZERO_ERROR); + assertEquals("unitUsage() en-US road", "1,000 ft", formattedNum.toString(status)); + status.errIfFailureAndReset("unitUsage() en-US road, toString(...)"); + U_ASSERT(status == U_ZERO_ERROR); assertFormatDescendingBig( u"unitUsage() en-US road", u"measure-unit/length-meter usage/road", @@ -746,9 +762,10 @@ void NumberFormatterApiTest::unitUsage() { u"54 mi", u"5.4 mi", u"0.54 mi", - u"288 ft", - u"29 ft", + u"300 ft", + u"30 ft", u"0 ft"); + } void NumberFormatterApiTest::unitUsageErrorCodes() { From 3f417677905d44beaed97c4f524cf0af06d8e781 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 14:57:48 +0200 Subject: [PATCH 02/25] Officially move the initNumberSkeletons call into parseSkeleton. --- icu4c/source/i18n/number_skeletons.cpp | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index eeefadfe3cc..454608765e8 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -447,11 +447,6 @@ UnlocalizedNumberFormatter skeleton::create( perror->postContext[0] = 0; } - umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); - if (U_FAILURE(status)) { - return {}; - } - int32_t errOffset; MacroProps macros = parseSkeleton(skeletonString, errOffset, status); if (U_SUCCESS(status)) { @@ -482,14 +477,10 @@ UnicodeString skeleton::generate(const MacroProps& macros, UErrorCode& status) { MacroProps skeleton::parseSkeleton( const UnicodeString& skeletonString, int32_t& errOffset, UErrorCode& status) { - U_ASSERT(U_SUCCESS(status)); - - // FIXME (DO NOT SUBMIT): In this PR, we consider calling parseSkeleton - // directly rather than going through create(). With the first call, we've - // missed initNumberSkeletons though. Do we want to go through create, which - // is just a little bit wasteful, or do we want to find an alternative way - // to ensure initNumberSkeletons has been called? umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); + if (U_FAILURE(status)) { + return MacroProps(); + } U_ASSERT(kSerializedStemTrie != nullptr); From 461d5ea9505340283e4c9d49d5ddfb5f7c16fbea Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 15:29:42 +0200 Subject: [PATCH 03/25] Back-propagate a U_ASSERT(U_SUCCESS(status)), and also add to parseStem. --- icu4c/source/i18n/number_skeletons.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index 454608765e8..a4deacd8357 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -478,6 +478,9 @@ UnicodeString skeleton::generate(const MacroProps& macros, UErrorCode& status) { MacroProps skeleton::parseSkeleton( const UnicodeString& skeletonString, int32_t& errOffset, UErrorCode& status) { umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); + + // parseStem and parseOption expect U_SUCCESS(status). We check status here + // and we return as soon as U_FAILURE(status) becomes true. if (U_FAILURE(status)) { return MacroProps(); } @@ -589,6 +592,8 @@ MacroProps skeleton::parseSkeleton( ParseState skeleton::parseStem(const StringSegment& segment, const UCharsTrie& stemTrie, SeenMacroProps& seen, MacroProps& macros, UErrorCode& status) { + U_ASSERT(U_SUCCESS(status)); + // First check for "blueprint" stems, which start with a "signal char" switch (segment.charAt(0)) { case u'.': @@ -767,6 +772,7 @@ skeleton::parseStem(const StringSegment& segment, const UCharsTrie& stemTrie, Se ParseState skeleton::parseOption(ParseState stem, const StringSegment& segment, MacroProps& macros, UErrorCode& status) { + U_ASSERT(U_SUCCESS(status)); ///// Required options: ///// @@ -995,6 +1001,7 @@ blueprint_helpers::generateCurrencyOption(const CurrencyUnit& currency, UnicodeS void blueprint_helpers::parseMeasureUnitOption(const StringSegment& segment, MacroProps& macros, UErrorCode& status) { + U_ASSERT(U_SUCCESS(status)); const UnicodeString stemString = segment.toTempUnicodeString(); // NOTE: The category (type) of the unit is guaranteed to be a valid subtag (alphanumeric) @@ -1010,7 +1017,6 @@ void blueprint_helpers::parseMeasureUnitOption(const StringSegment& segment, Mac } // Need to do char <-> UChar conversion... - U_ASSERT(U_SUCCESS(status)); CharString type; SKELETON_UCHAR_TO_CHAR(type, stemString, 0, firstHyphen, status); CharString subType; From e254b05cec6911b82f4d0c5b61d5510b93bc145d Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 17:40:43 +0200 Subject: [PATCH 04/25] Implement overrideable rounding behaviour --- icu4c/source/i18n/number_formatimpl.cpp | 10 +++++- icu4c/source/i18n/number_roundingutils.h | 3 ++ icu4c/source/i18n/number_usageprefs.cpp | 7 ++-- icu4c/source/test/intltest/numbertest.h | 1 + icu4c/source/test/intltest/numbertest_api.cpp | 34 ++++++++++++++++--- 5 files changed, 47 insertions(+), 8 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index b7aee30225f..26ae043d1f3 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -249,6 +249,7 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, // Rounding strategy Precision precision; + bool defaultRounding = false; if (!macros.precision.isBogus()) { precision = macros.precision; } else if (macros.notation.fType == Notation::NTN_COMPACT) { @@ -256,6 +257,7 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, } else if (isCurrency) { precision = Precision::currency(UCURR_USAGE_STANDARD); } else { + defaultRounding = true; precision = Precision::maxFraction(6); } UNumberFormatRoundingMode roundingMode; @@ -265,7 +267,13 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, // Temporary until ICU 64 roundingMode = precision.fRoundingMode; } - fMicros.rounder = {precision, roundingMode, currency, status}; + if (defaultRounding && macros.usage.isSet()) { + // If defaultRounding and usage is set, we lave the rounding up to + // UsagePrefsHandler. + U_ASSERT(fUsagePrefsHandler.getAlias() != nullptr); + } else { + fMicros.rounder = {precision, roundingMode, currency, status}; + } if (U_FAILURE(status)) { return nullptr; } diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index f672641cf61..4207ab274b5 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -163,6 +163,9 @@ class RoundingImpl { /** Required for ScientificFormatter */ bool isSignificantDigits() const; + /** Returns true if this is a default do-nothing instance */ + bool isPassThrough() const { return fPassThrough; } + /** * Rounding endpoint used by Engineering and Compact notation. Chooses the most appropriate multiplier (magnitude * adjustment), applies the adjustment, rounds, and returns the chosen multiplier. diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index d239764b4b9..c4c7f820d62 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -110,9 +110,10 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m quantity.setToDouble(routedUnits[0]->getNumber().getDouble()); UnicodeString precisionSkeleton = routed.precision; - // TODO(icu-units/icu#13): If the programmer specified a precision, use - // that. - if (precisionSkeleton.length() > 0) { + if (!micros.rounder.isPassThrough()) { + // Do nothing: we already have a rounder, so we don't use + // precisionSkeleton or a default "usage-appropriate" rounder. + } else if (precisionSkeleton.length() > 0) { CharString csPrecisionSkeleton; UErrorCode csErrCode = U_ZERO_ERROR; csPrecisionSkeleton.appendInvariantChars(precisionSkeleton, csErrCode); diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h index b5f52ccc553..9ed6877b0a4 100644 --- a/icu4c/source/test/intltest/numbertest.h +++ b/icu4c/source/test/intltest/numbertest.h @@ -56,6 +56,7 @@ class NumberFormatterApiTest : public IntlTestWithFieldPosition { void unitCompoundMeasure(); void unitUsage(); void unitUsageErrorCodes(); + void unitUsageSkeletons(); void unitCurrency(); void unitPercent(); void percentParity(); diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index a04c2772f53..a9339aa8625 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -77,6 +77,7 @@ void NumberFormatterApiTest::runIndexedTest(int32_t index, UBool exec, const cha TESTCASE_AUTO(unitCompoundMeasure); TESTCASE_AUTO(unitUsage); TESTCASE_AUTO(unitUsageErrorCodes); + TESTCASE_AUTO(unitUsageSkeletons); TESTCASE_AUTO(unitCurrency); TESTCASE_AUTO(unitPercent); if (!quick) { @@ -688,7 +689,7 @@ void NumberFormatterApiTest::unitUsage() { FormattedNumber formattedNum; formatter = unloc_formatter.locale("en-ZA"); - formattedNum = formatter.formatDouble(304, status); + formattedNum = formatter.formatDouble(321, status); status.errIfFailureAndReset("unitUsage() en-ZA road, formatDouble(...)"); assertTrue(UnicodeString("unitUsage() en-ZA road, got outputUnit: \"") + formattedNum.getOutputUnit(status).getIdentifier() + "\"", @@ -711,7 +712,7 @@ void NumberFormatterApiTest::unitUsage() { u"0 m"); formatter = unloc_formatter.locale("en-GB"); - formattedNum = formatter.formatDouble(304, status); + formattedNum = formatter.formatDouble(321, status); status.errIfFailureAndReset("unitUsage() en-GB road, formatDouble(...)"); U_ASSERT(status == U_ZERO_ERROR); assertTrue(UnicodeString("unitUsage() en-GB road, got outputUnit: \"") + @@ -739,7 +740,7 @@ void NumberFormatterApiTest::unitUsage() { u"0 yd"); formatter = unloc_formatter.locale("en-US"); - formattedNum = formatter.formatDouble(304, status); + formattedNum = formatter.formatDouble(321, status); status.errIfFailureAndReset("unitUsage() en-US road, formatDouble(...)"); U_ASSERT(status == U_ZERO_ERROR); assertTrue(UnicodeString("unitUsage() en-US road, got outputUnit: \"") + @@ -747,7 +748,7 @@ void NumberFormatterApiTest::unitUsage() { MeasureUnit::getFoot() == formattedNum.getOutputUnit(status)); status.errIfFailureAndReset("unitUsage() en-US road, getOutputUnit(...)"); U_ASSERT(status == U_ZERO_ERROR); - assertEquals("unitUsage() en-US road", "1,000 ft", formattedNum.toString(status)); + assertEquals("unitUsage() en-US road", "1,050 ft", formattedNum.toString(status)); status.errIfFailureAndReset("unitUsage() en-US road, toString(...)"); U_ASSERT(status == U_ZERO_ERROR); assertFormatDescendingBig( @@ -789,6 +790,31 @@ void NumberFormatterApiTest::unitUsageErrorCodes() { status.assertSuccess(); } +void NumberFormatterApiTest::unitUsageSkeletons() { + IcuTestErrorCode status(*this, "unitUsageSkeletons()"); + + auto formatter = NumberFormatter::forSkeleton("usage/road unit/meter", status).locale("en-ZA"); + auto result = formatter.formatDouble(321, status).toString(status); + status.assertSuccess(); + assertEquals("unitUsageSkeletons() usage/road unit/meter en-ZA", "300 m", result); + + // FIXME: hey Shane, is modifying a LocalizedNumberFormatter safe? I see + // fUnsafeCallCount is counted on the LocalizedNumberFormatter, I'm + // wondering if a Localized formatter might get compiled, and then no longer + // respond to changes in fMarcos: + // + // Override the formatter's precision + formatter = formatter.precision(Precision::maxSignificantDigits(2)); + result = formatter.formatDouble(321, status).toString(status); + status.assertSuccess(); + assertEquals("unitUsageSkeletons() precision override (usage/road unit/meter en-ZA)", "320 m", + result); + + // TODO: ScientificHandler::processQuantity sets mciros.rounder to + // passThrough. Test whether we might have handlers flip-flopping this, if + // we are doing both usage & scientific. +} + void NumberFormatterApiTest::unitCompoundMeasure() { assertFormatDescending( u"Meters Per Second Short (unit that simplifies) and perUnit method", From f0ec4880462c7599b8a7ac8a99fce83d09ff0855 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 17:44:00 +0200 Subject: [PATCH 05/25] Move scientific unit tests to a different branch, rounder WAI. --- icu4c/source/test/intltest/numbertest_api.cpp | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index a9339aa8625..54eb954ae05 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -793,26 +793,18 @@ void NumberFormatterApiTest::unitUsageErrorCodes() { void NumberFormatterApiTest::unitUsageSkeletons() { IcuTestErrorCode status(*this, "unitUsageSkeletons()"); - auto formatter = NumberFormatter::forSkeleton("usage/road unit/meter", status).locale("en-ZA"); - auto result = formatter.formatDouble(321, status).toString(status); + // Default >300m road preference skeletons round to 50m + auto roadFormatter = NumberFormatter::forSkeleton("usage/road unit/meter", status).locale("en-ZA"); + auto result = roadFormatter.formatDouble(321, status).toString(status); status.assertSuccess(); assertEquals("unitUsageSkeletons() usage/road unit/meter en-ZA", "300 m", result); - // FIXME: hey Shane, is modifying a LocalizedNumberFormatter safe? I see - // fUnsafeCallCount is counted on the LocalizedNumberFormatter, I'm - // wondering if a Localized formatter might get compiled, and then no longer - // respond to changes in fMarcos: - // - // Override the formatter's precision - formatter = formatter.precision(Precision::maxSignificantDigits(2)); - result = formatter.formatDouble(321, status).toString(status); + // Override the roadFormatter's precision + roadFormatter = roadFormatter.precision(Precision::maxSignificantDigits(2)); + result = roadFormatter.formatDouble(321, status).toString(status); status.assertSuccess(); assertEquals("unitUsageSkeletons() precision override (usage/road unit/meter en-ZA)", "320 m", result); - - // TODO: ScientificHandler::processQuantity sets mciros.rounder to - // passThrough. Test whether we might have handlers flip-flopping this, if - // we are doing both usage & scientific. } void NumberFormatterApiTest::unitCompoundMeasure() { From 0589bc95b46a7e5ac0ae565651faa34430232388 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 19:58:05 +0200 Subject: [PATCH 06/25] dependencies.txt: merge numberformatter and number_skeletons groups. :-( --- icu4c/source/test/depstest/dependencies.txt | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 58d82cd8974..a2c35e04eca 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -869,7 +869,7 @@ library: i18n dayperiodrules listformatter formatting formattable_cnv regex regex_cnv translit - double_conversion number_representation number_output numberformatter number_skeletons numberparser + double_conversion number_representation number_output numberformatter numberparser units_extra unitsformatter universal_time_scale uclean_i18n @@ -995,16 +995,13 @@ group: numberformatter number_scientific.o number_usageprefs.o currpinf.o dcfmtsym.o numsys.o numrange_fluent.o numrange_impl.o + # Number skeleton support: + number_skeletons.o number_capi.o number_asformat.o numrange_capi.o deps decnumber double_conversion formattable units unitsformatter number_representation number_output uclean_i18n common - -group: number_skeletons - # Number skeleton support; separated from numberformatter - number_skeletons.o number_capi.o number_asformat.o numrange_capi.o - deps - numberformatter + # Used in number skeletons: units_extra group: numberparser @@ -1047,7 +1044,7 @@ group: formatting # messageformat choicfmt.o msgfmt.o plurfmt.o selfmt.o umsg.o deps - decnumber formattable format units numberformatter number_skeletons numberparser + decnumber formattable format units numberformatter numberparser formatted_value_sbimpl listformatter dayperiodrules From 95c0a17d19d280f9694aa7899c63ab62ee969b29 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 18 Aug 2020 20:44:02 +0200 Subject: [PATCH 07/25] Revert "dependencies.txt: merge numberformatter and number_skeletons groups. :-(" This reverts commit 0589bc95b46a7e5ac0ae565651faa34430232388. --- icu4c/source/test/depstest/dependencies.txt | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index a2c35e04eca..58d82cd8974 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -869,7 +869,7 @@ library: i18n dayperiodrules listformatter formatting formattable_cnv regex regex_cnv translit - double_conversion number_representation number_output numberformatter numberparser + double_conversion number_representation number_output numberformatter number_skeletons numberparser units_extra unitsformatter universal_time_scale uclean_i18n @@ -995,13 +995,16 @@ group: numberformatter number_scientific.o number_usageprefs.o currpinf.o dcfmtsym.o numsys.o numrange_fluent.o numrange_impl.o - # Number skeleton support: - number_skeletons.o number_capi.o number_asformat.o numrange_capi.o deps decnumber double_conversion formattable units unitsformatter number_representation number_output uclean_i18n common - # Used in number skeletons: + +group: number_skeletons + # Number skeleton support; separated from numberformatter + number_skeletons.o number_capi.o number_asformat.o numrange_capi.o + deps + numberformatter units_extra group: numberparser @@ -1044,7 +1047,7 @@ group: formatting # messageformat choicfmt.o msgfmt.o plurfmt.o selfmt.o umsg.o deps - decnumber formattable format units numberformatter numberparser + decnumber formattable format units numberformatter number_skeletons numberparser formatted_value_sbimpl listformatter dayperiodrules From 5ce53b4c7273f6b69ad0be4e0fa598646bd409aa Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 03:12:26 +0200 Subject: [PATCH 08/25] A relatively minimal but slightly haphazard code shuffle to fix dependencies.txt. --- icu4c/source/i18n/number_fluent.cpp | 32 ++--- icu4c/source/i18n/number_formatimpl.cpp | 3 - icu4c/source/i18n/number_rounding.cpp | 35 +++++ icu4c/source/i18n/number_skeletons.cpp | 71 ++++----- icu4c/source/i18n/number_skeletons.h | 18 +++ icu4c/source/i18n/number_usageprefs.cpp | 152 +++++++++++++++++--- icu4c/source/i18n/unicode/numberformatter.h | 9 +- icu4c/source/test/depstest/dependencies.txt | 23 ++- 8 files changed, 251 insertions(+), 92 deletions(-) diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp index 579f3062142..4a0e31a45d3 100644 --- a/icu4c/source/i18n/number_fluent.cpp +++ b/icu4c/source/i18n/number_fluent.cpp @@ -546,9 +546,9 @@ SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { return *this; } -SymbolsWrapper::~SymbolsWrapper() { - doCleanup(); -} +// SymbolsWrapper::~SymbolsWrapper() { +// doCleanup(); +// } void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { doCleanup(); @@ -604,19 +604,19 @@ void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { } } -void SymbolsWrapper::doCleanup() { - switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - delete fPtr.dfs; - break; - case SYMPTR_NS: - delete fPtr.ns; - break; - } -} +// void SymbolsWrapper::doCleanup() { +// switch (fType) { +// case SYMPTR_NONE: +// // No action necessary +// break; +// case SYMPTR_DFS: +// delete fPtr.dfs; +// break; +// case SYMPTR_NS: +// delete fPtr.ns; +// break; +// } +// } bool SymbolsWrapper::isDecimalFormatSymbols() const { return fType == SYMPTR_DFS; diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index 26ae043d1f3..98281508278 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -25,9 +25,6 @@ using namespace icu::number; using namespace icu::number::impl; -MicroPropsGenerator::~MicroPropsGenerator() = default; - - NumberFormatterImpl::NumberFormatterImpl(const MacroProps& macros, UErrorCode& status) : NumberFormatterImpl(macros, true, status) { } diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index ee0a6985f1b..f76d0d16cc5 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -5,13 +5,16 @@ #if !UCONFIG_NO_FORMATTING +#include "charstr.h" #include "uassert.h" #include "unicode/numberformatter.h" #include "number_types.h" #include "number_decimalquantity.h" #include "double-conversion.h" #include "number_roundingutils.h" +#include "number_skeletons.h" #include "putilimp.h" +#include "string_segment.h" using namespace icu; using namespace icu::number; @@ -19,6 +22,38 @@ using namespace icu::number::impl; using double_conversion::DoubleToStringConverter; +using icu::StringSegment; + +void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, + UErrorCode& status) { + // Need to do char <-> UChar conversion... + U_ASSERT(U_SUCCESS(status)); + CharString buffer; + SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); + + // Utilize DecimalQuantity/decNumber to parse this for us. + DecimalQuantity dq; + UErrorCode localStatus = U_ZERO_ERROR; + dq.setToDecNumber({buffer.data(), buffer.length()}, localStatus); + if (U_FAILURE(localStatus)) { + // throw new SkeletonSyntaxException("Invalid rounding increment", segment, e); + status = U_NUMBER_SKELETON_SYNTAX_ERROR; + return; + } + double increment = dq.toDouble(); + + // We also need to figure out how many digits. Do a brute force string operation. + int decimalOffset = 0; + while (decimalOffset < segment.length() && segment.charAt(decimalOffset) != '.') { + decimalOffset++; + } + if (decimalOffset == segment.length()) { + macros.precision = Precision::increment(increment); + } else { + int32_t fractionLength = segment.length() - decimalOffset - 1; + macros.precision = Precision::increment(increment).withMinFraction(fractionLength); + } +} namespace { diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index a4deacd8357..1f5ec47049f 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -152,21 +152,6 @@ UPRV_BLOCK_MACRO_BEGIN { \ } UPRV_BLOCK_MACRO_END -#define SKELETON_UCHAR_TO_CHAR(dest, src, start, end, status) (void)(dest); \ -UPRV_BLOCK_MACRO_BEGIN { \ - UErrorCode conversionStatus = U_ZERO_ERROR; \ - (dest).appendInvariantChars({FALSE, (src).getBuffer() + (start), (end) - (start)}, conversionStatus); \ - if (conversionStatus == U_INVARIANT_CONVERSION_ERROR) { \ - /* Don't propagate the invariant conversion error; it is a skeleton syntax error */ \ - (status) = U_NUMBER_SKELETON_SYNTAX_ERROR; \ - return; \ - } else if (U_FAILURE(conversionStatus)) { \ - (status) = conversionStatus; \ - return; \ - } \ -} UPRV_BLOCK_MACRO_END - - } // anonymous namespace @@ -1345,36 +1330,36 @@ bool blueprint_helpers::parseFracSigOption(const StringSegment& segment, MacroPr return true; } -void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, - UErrorCode& status) { - // Need to do char <-> UChar conversion... - U_ASSERT(U_SUCCESS(status)); - CharString buffer; - SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); +// void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, +// UErrorCode& status) { +// // Need to do char <-> UChar conversion... +// U_ASSERT(U_SUCCESS(status)); +// CharString buffer; +// SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); - // Utilize DecimalQuantity/decNumber to parse this for us. - DecimalQuantity dq; - UErrorCode localStatus = U_ZERO_ERROR; - dq.setToDecNumber({buffer.data(), buffer.length()}, localStatus); - if (U_FAILURE(localStatus)) { - // throw new SkeletonSyntaxException("Invalid rounding increment", segment, e); - status = U_NUMBER_SKELETON_SYNTAX_ERROR; - return; - } - double increment = dq.toDouble(); +// // Utilize DecimalQuantity/decNumber to parse this for us. +// DecimalQuantity dq; +// UErrorCode localStatus = U_ZERO_ERROR; +// dq.setToDecNumber({buffer.data(), buffer.length()}, localStatus); +// if (U_FAILURE(localStatus)) { +// // throw new SkeletonSyntaxException("Invalid rounding increment", segment, e); +// status = U_NUMBER_SKELETON_SYNTAX_ERROR; +// return; +// } +// double increment = dq.toDouble(); - // We also need to figure out how many digits. Do a brute force string operation. - int decimalOffset = 0; - while (decimalOffset < segment.length() && segment.charAt(decimalOffset) != '.') { - decimalOffset++; - } - if (decimalOffset == segment.length()) { - macros.precision = Precision::increment(increment); - } else { - int32_t fractionLength = segment.length() - decimalOffset - 1; - macros.precision = Precision::increment(increment).withMinFraction(fractionLength); - } -} +// // We also need to figure out how many digits. Do a brute force string operation. +// int decimalOffset = 0; +// while (decimalOffset < segment.length() && segment.charAt(decimalOffset) != '.') { +// decimalOffset++; +// } +// if (decimalOffset == segment.length()) { +// macros.precision = Precision::increment(increment); +// } else { +// int32_t fractionLength = segment.length() - decimalOffset - 1; +// macros.precision = Precision::increment(increment).withMinFraction(fractionLength); +// } +// } void blueprint_helpers::generateIncrementOption(double increment, int32_t trailingZeros, UnicodeString& sb, UErrorCode&) { diff --git a/icu4c/source/i18n/number_skeletons.h b/icu4c/source/i18n/number_skeletons.h index 313c7ac54c2..b29b7f9960b 100644 --- a/icu4c/source/i18n/number_skeletons.h +++ b/icu4c/source/i18n/number_skeletons.h @@ -355,6 +355,24 @@ struct SeenMacroProps { bool scale = false; }; +namespace { + +#define SKELETON_UCHAR_TO_CHAR(dest, src, start, end, status) (void)(dest); \ +UPRV_BLOCK_MACRO_BEGIN { \ + UErrorCode conversionStatus = U_ZERO_ERROR; \ + (dest).appendInvariantChars({FALSE, (src).getBuffer() + (start), (end) - (start)}, conversionStatus); \ + if (conversionStatus == U_INVARIANT_CONVERSION_ERROR) { \ + /* Don't propagate the invariant conversion error; it is a skeleton syntax error */ \ + (status) = U_NUMBER_SKELETON_SYNTAX_ERROR; \ + return; \ + } else if (U_FAILURE(conversionStatus)) { \ + (status) = conversionStatus; \ + return; \ + } \ +} UPRV_BLOCK_MACRO_END + +} // namespace + } // namespace impl } // namespace number U_NAMESPACE_END diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index c4c7f820d62..727aa4eabff 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -20,7 +20,132 @@ using namespace icu::number; using namespace icu::number::impl; -using icu::number::impl::skeleton::parseSkeleton; +using icu::StringSegment; + +MicroPropsGenerator::~MicroPropsGenerator() = default; + +// SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { +// doCopyFrom(other); +// } + +// SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { +// doMoveFrom(std::move(src)); +// } + +// SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { +// if (this == &other) { +// return *this; +// } +// doCleanup(); +// doCopyFrom(other); +// return *this; +// } + +// SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { +// if (this == &src) { +// return *this; +// } +// doCleanup(); +// doMoveFrom(std::move(src)); +// return *this; +// } + +SymbolsWrapper::~SymbolsWrapper() { + doCleanup(); +} + +// void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { +// doCleanup(); +// fType = SYMPTR_DFS; +// fPtr.dfs = new DecimalFormatSymbols(dfs); +// } + +// void SymbolsWrapper::setTo(const NumberingSystem* ns) { +// doCleanup(); +// fType = SYMPTR_NS; +// fPtr.ns = ns; +// } + +// void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { +// fType = other.fType; +// switch (fType) { +// case SYMPTR_NONE: +// // No action necessary +// break; +// case SYMPTR_DFS: +// // Memory allocation failures are exposed in copyErrorTo() +// if (other.fPtr.dfs != nullptr) { +// fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); +// } else { +// fPtr.dfs = nullptr; +// } +// break; +// case SYMPTR_NS: +// // Memory allocation failures are exposed in copyErrorTo() +// if (other.fPtr.ns != nullptr) { +// fPtr.ns = new NumberingSystem(*other.fPtr.ns); +// } else { +// fPtr.ns = nullptr; +// } +// break; +// } +// } + +// void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { +// fType = src.fType; +// switch (fType) { +// case SYMPTR_NONE: +// // No action necessary +// break; +// case SYMPTR_DFS: +// fPtr.dfs = src.fPtr.dfs; +// src.fPtr.dfs = nullptr; +// break; +// case SYMPTR_NS: +// fPtr.ns = src.fPtr.ns; +// src.fPtr.ns = nullptr; +// break; +// } +// } + +void SymbolsWrapper::doCleanup() { + switch (fType) { + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + delete fPtr.dfs; + break; + case SYMPTR_NS: + delete fPtr.ns; + break; + } +} + +// bool SymbolsWrapper::isDecimalFormatSymbols() const { +// return fType == SYMPTR_DFS; +// } + +// bool SymbolsWrapper::isNumberingSystem() const { +// return fType == SYMPTR_NS; +// } + +Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status) { + if (U_FAILURE(status)) { + return {}; + } + constexpr int32_t kSkelPrefixLen = 20; + if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { + status = U_INVALID_FORMAT_ERROR; + return {}; + } + U_ASSERT(precisionSkeleton[kSkelPrefixLen-1] == u'/'); + StringSegment segment(precisionSkeleton, false); + segment.adjustOffset(kSkelPrefixLen); + MacroProps macros; + blueprint_helpers::parseIncrementOption(segment, macros, status); + return macros.precision; +} // Copy constructor Usage::Usage(const Usage &other) : fUsage(nullptr), fLength(other.fLength), fError(other.fError) { @@ -110,6 +235,10 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m quantity.setToDouble(routedUnits[0]->getNumber().getDouble()); UnicodeString precisionSkeleton = routed.precision; + // TODO: Is it okay if this is kDefaultMode? + // Otherwise it was: unitPrefMacros.roundingMode or precision.fRoundingMode; + UNumberFormatRoundingMode roundingMode = kDefaultMode; + CurrencyUnit currency(u"", status); if (!micros.rounder.isPassThrough()) { // Do nothing: we already have a rounder, so we don't use // precisionSkeleton or a default "usage-appropriate" rounder. @@ -119,30 +248,13 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m csPrecisionSkeleton.appendInvariantChars(precisionSkeleton, csErrCode); // Parse skeleton, collect results - int32_t errOffset; + // int32_t errOffset; // int32_t errOffset = 0; U_ASSERT(U_SUCCESS(status)); - MacroProps unitPrefMacros = parseSkeleton(precisionSkeleton, errOffset, status); - Precision precision; - UNumberFormatRoundingMode roundingMode; - if (U_FAILURE(status)) { - return; - } - precision = unitPrefMacros.precision; - if (unitPrefMacros.roundingMode != kDefaultMode) { - roundingMode = unitPrefMacros.roundingMode; - } else { - // TODO: "Temporary until ICU 64" in number_formatimpl.cpp? Clarify! - roundingMode = precision.fRoundingMode; - } - CurrencyUnit currency(u"", status); + Precision precision = parseSkeletonToPrecision(precisionSkeleton, status); micros.rounder = {precision, roundingMode, currency, status}; } else { Precision precision = Precision::integer().withMinDigits(2); - UNumberFormatRoundingMode roundingMode; - // Temporary until ICU 64? - roundingMode = precision.fRoundingMode; - CurrencyUnit currency(u"", status); micros.rounder = {precision, roundingMode, currency, status}; } } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index b17e25373e6..b6b786a70cf 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -717,9 +717,9 @@ class U_I18N_API Precision : public UMemory { Precision(UErrorCode errorCode) : fType(RND_ERROR) { fUnion.errorCode = errorCode; } - +public: // Constructor needed by parseSkeletonToPrecision: Precision() : fType(RND_BOGUS) {} - +private: bool isBogus() const { return fType == RND_BOGUS; } @@ -769,10 +769,7 @@ class U_I18N_API Precision : public UMemory { // To allow access to the skeleton generation code: friend class impl::GeneratorHelpers; - // TODO(units): revisit when UnitsRouter is changed: do we still need this - // once Precision is returned by UnitsRouter? For now, we allow access to - // Precision constructor from UsagePrefsHandler: - friend class impl::UsagePrefsHandler; + // friend class impl::UsagePrefsHandler; }; /** diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 58d82cd8974..895e83f2488 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -869,7 +869,8 @@ library: i18n dayperiodrules listformatter formatting formattable_cnv regex regex_cnv translit - double_conversion number_representation number_output numberformatter number_skeletons numberparser + double_conversion number_representation number_output numberformatter + number_skeletons numberformatter_microprops numberparser units_extra unitsformatter universal_time_scale uclean_i18n @@ -989,17 +990,31 @@ group: numberformatter number_decimfmtprops.o number_fluent.o number_formatimpl.o number_grouping.o number_integerwidth.o number_longnames.o - number_mapper.o number_modifiers.o number_multiplier.o + number_mapper.o number_modifiers.o number_notation.o number_padding.o - number_patternmodifier.o number_patternstring.o number_rounding.o - number_scientific.o number_usageprefs.o + number_patternmodifier.o number_patternstring.o + number_scientific.o currpinf.o dcfmtsym.o numsys.o numrange_fluent.o numrange_impl.o deps decnumber double_conversion formattable units unitsformatter number_representation number_output + numberformatter_microprops uclean_i18n common +group: numberformatter_microprops + number_multiplier.o + number_usageprefs.o + deps + number_rounding + unitsformatter + +group: number_rounding + number_rounding.o + deps + currency + number_representation + group: number_skeletons # Number skeleton support; separated from numberformatter number_skeletons.o number_capi.o number_asformat.o numrange_capi.o From f07f3f091d3fd8ff6fb84e1cee6eeac95f0b4dea Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 03:35:22 +0200 Subject: [PATCH 09/25] Move to number_symbolswrapper.cpp the SymbolsWrapper code from number_usageprefs.cpp --- icu4c/source/i18n/i18n.vcxproj | 4 +- icu4c/source/i18n/i18n.vcxproj.filters | 12 ++ icu4c/source/i18n/number_symbolswrapper.cpp | 127 ++++++++++++++++++++ icu4c/source/i18n/number_usageprefs.cpp | 108 ----------------- icu4c/source/i18n/sources.txt | 1 + icu4c/source/test/depstest/dependencies.txt | 7 ++ 6 files changed, 150 insertions(+), 109 deletions(-) create mode 100644 icu4c/source/i18n/number_symbolswrapper.cpp diff --git a/icu4c/source/i18n/i18n.vcxproj b/icu4c/source/i18n/i18n.vcxproj index cef0ad344ee..9df696fc72f 100644 --- a/icu4c/source/i18n/i18n.vcxproj +++ b/icu4c/source/i18n/i18n.vcxproj @@ -219,6 +219,7 @@ + @@ -487,13 +488,14 @@ - + + diff --git a/icu4c/source/i18n/i18n.vcxproj.filters b/icu4c/source/i18n/i18n.vcxproj.filters index 4e65f29a2c4..83409f013d2 100644 --- a/icu4c/source/i18n/i18n.vcxproj.filters +++ b/icu4c/source/i18n/i18n.vcxproj.filters @@ -591,6 +591,9 @@ formatting + + formatting + formatting @@ -606,6 +609,9 @@ formatting + + formatting + formatting @@ -917,6 +923,9 @@ formatting + + formatting + formatting @@ -932,6 +941,9 @@ formatting + + formatting + formatting diff --git a/icu4c/source/i18n/number_symbolswrapper.cpp b/icu4c/source/i18n/number_symbolswrapper.cpp new file mode 100644 index 00000000000..9a819721500 --- /dev/null +++ b/icu4c/source/i18n/number_symbolswrapper.cpp @@ -0,0 +1,127 @@ +// © 2020 and later: Unicode, Inc. and others. +// License & terms of use: http://www.unicode.org/copyright.html + +#include "number_microprops.h" +#include "unicode/numberformatter.h" + +using namespace icu; +using namespace icu::number; +using namespace icu::number::impl; + +MicroPropsGenerator::~MicroPropsGenerator() = default; + +// SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { +// doCopyFrom(other); +// } + +// SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { +// doMoveFrom(std::move(src)); +// } + +// SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { +// if (this == &other) { +// return *this; +// } +// doCleanup(); +// doCopyFrom(other); +// return *this; +// } + +// SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { +// if (this == &src) { +// return *this; +// } +// doCleanup(); +// doMoveFrom(std::move(src)); +// return *this; +// } + +SymbolsWrapper::~SymbolsWrapper() { + doCleanup(); +} + +// void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { +// doCleanup(); +// fType = SYMPTR_DFS; +// fPtr.dfs = new DecimalFormatSymbols(dfs); +// } + +// void SymbolsWrapper::setTo(const NumberingSystem* ns) { +// doCleanup(); +// fType = SYMPTR_NS; +// fPtr.ns = ns; +// } + +// void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { +// fType = other.fType; +// switch (fType) { +// case SYMPTR_NONE: +// // No action necessary +// break; +// case SYMPTR_DFS: +// // Memory allocation failures are exposed in copyErrorTo() +// if (other.fPtr.dfs != nullptr) { +// fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); +// } else { +// fPtr.dfs = nullptr; +// } +// break; +// case SYMPTR_NS: +// // Memory allocation failures are exposed in copyErrorTo() +// if (other.fPtr.ns != nullptr) { +// fPtr.ns = new NumberingSystem(*other.fPtr.ns); +// } else { +// fPtr.ns = nullptr; +// } +// break; +// } +// } + +// void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { +// fType = src.fType; +// switch (fType) { +// case SYMPTR_NONE: +// // No action necessary +// break; +// case SYMPTR_DFS: +// fPtr.dfs = src.fPtr.dfs; +// src.fPtr.dfs = nullptr; +// break; +// case SYMPTR_NS: +// fPtr.ns = src.fPtr.ns; +// src.fPtr.ns = nullptr; +// break; +// } +// } + +void SymbolsWrapper::doCleanup() { + switch (fType) { + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + delete fPtr.dfs; + break; + case SYMPTR_NS: + delete fPtr.ns; + break; + } +} + +// bool SymbolsWrapper::isDecimalFormatSymbols() const { +// return fType == SYMPTR_DFS; +// } + +// bool SymbolsWrapper::isNumberingSystem() const { +// return fType == SYMPTR_NS; +// } + +// const DecimalFormatSymbols* SymbolsWrapper::getDecimalFormatSymbols() const { +// U_ASSERT(fType == SYMPTR_DFS); +// return fPtr.dfs; +// } + +// const NumberingSystem* SymbolsWrapper::getNumberingSystem() const { +// U_ASSERT(fType == SYMPTR_NS); +// return fPtr.ns; +// } diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 727aa4eabff..8ccdac8ea27 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -22,114 +22,6 @@ using namespace icu::number; using namespace icu::number::impl; using icu::StringSegment; -MicroPropsGenerator::~MicroPropsGenerator() = default; - -// SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { -// doCopyFrom(other); -// } - -// SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { -// doMoveFrom(std::move(src)); -// } - -// SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { -// if (this == &other) { -// return *this; -// } -// doCleanup(); -// doCopyFrom(other); -// return *this; -// } - -// SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { -// if (this == &src) { -// return *this; -// } -// doCleanup(); -// doMoveFrom(std::move(src)); -// return *this; -// } - -SymbolsWrapper::~SymbolsWrapper() { - doCleanup(); -} - -// void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { -// doCleanup(); -// fType = SYMPTR_DFS; -// fPtr.dfs = new DecimalFormatSymbols(dfs); -// } - -// void SymbolsWrapper::setTo(const NumberingSystem* ns) { -// doCleanup(); -// fType = SYMPTR_NS; -// fPtr.ns = ns; -// } - -// void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { -// fType = other.fType; -// switch (fType) { -// case SYMPTR_NONE: -// // No action necessary -// break; -// case SYMPTR_DFS: -// // Memory allocation failures are exposed in copyErrorTo() -// if (other.fPtr.dfs != nullptr) { -// fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); -// } else { -// fPtr.dfs = nullptr; -// } -// break; -// case SYMPTR_NS: -// // Memory allocation failures are exposed in copyErrorTo() -// if (other.fPtr.ns != nullptr) { -// fPtr.ns = new NumberingSystem(*other.fPtr.ns); -// } else { -// fPtr.ns = nullptr; -// } -// break; -// } -// } - -// void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { -// fType = src.fType; -// switch (fType) { -// case SYMPTR_NONE: -// // No action necessary -// break; -// case SYMPTR_DFS: -// fPtr.dfs = src.fPtr.dfs; -// src.fPtr.dfs = nullptr; -// break; -// case SYMPTR_NS: -// fPtr.ns = src.fPtr.ns; -// src.fPtr.ns = nullptr; -// break; -// } -// } - -void SymbolsWrapper::doCleanup() { - switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - delete fPtr.dfs; - break; - case SYMPTR_NS: - delete fPtr.ns; - break; - } -} - -// bool SymbolsWrapper::isDecimalFormatSymbols() const { -// return fType == SYMPTR_DFS; -// } - -// bool SymbolsWrapper::isNumberingSystem() const { -// return fType == SYMPTR_NS; -// } - Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status) { if (U_FAILURE(status)) { return {}; diff --git a/icu4c/source/i18n/sources.txt b/icu4c/source/i18n/sources.txt index 5e77d679aac..2d95da6cdf5 100644 --- a/icu4c/source/i18n/sources.txt +++ b/icu4c/source/i18n/sources.txt @@ -123,6 +123,7 @@ number_patternstring.cpp number_rounding.cpp number_scientific.cpp number_skeletons.cpp +number_symbolswrapper.cpp number_usageprefs.cpp number_utils.cpp numfmt.cpp diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 895e83f2488..d0d8c3ba342 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -1001,12 +1001,14 @@ group: numberformatter number_representation number_output numberformatter_microprops uclean_i18n common + number_symbolswrapper group: numberformatter_microprops number_multiplier.o number_usageprefs.o deps number_rounding + number_symbolswrapper unitsformatter group: number_rounding @@ -1022,6 +1024,11 @@ group: number_skeletons numberformatter units_extra +group: number_symbolswrapper + number_symbolswrapper.o + deps + platform + group: numberparser numparse_affixes.o numparse_compositions.o numparse_currency.o numparse_decimal.o numparse_impl.o numparse_parsednumber.o From 49dccd50877a179d3d6c72029d6742997de843de Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 03:56:16 +0200 Subject: [PATCH 10/25] number_symbolswrapper.cpp & co: thorough/consistent shuffling of code. --- icu4c/source/i18n/number_fluent.cpp | 117 ------------- icu4c/source/i18n/number_symbolswrapper.cpp | 174 ++++++++++---------- icu4c/source/i18n/number_types.h | 2 +- icu4c/source/test/depstest/dependencies.txt | 12 +- 4 files changed, 98 insertions(+), 207 deletions(-) diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp index 4a0e31a45d3..8569a36e5b2 100644 --- a/icu4c/source/i18n/number_fluent.cpp +++ b/icu4c/source/i18n/number_fluent.cpp @@ -520,123 +520,6 @@ LocalizedNumberFormatter UnlocalizedNumberFormatter::locale(const Locale& locale return LocalizedNumberFormatter(std::move(fMacros), locale); } -SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { - doCopyFrom(other); -} - -SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { - doMoveFrom(std::move(src)); -} - -SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { - if (this == &other) { - return *this; - } - doCleanup(); - doCopyFrom(other); - return *this; -} - -SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { - if (this == &src) { - return *this; - } - doCleanup(); - doMoveFrom(std::move(src)); - return *this; -} - -// SymbolsWrapper::~SymbolsWrapper() { -// doCleanup(); -// } - -void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { - doCleanup(); - fType = SYMPTR_DFS; - fPtr.dfs = new DecimalFormatSymbols(dfs); -} - -void SymbolsWrapper::setTo(const NumberingSystem* ns) { - doCleanup(); - fType = SYMPTR_NS; - fPtr.ns = ns; -} - -void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { - fType = other.fType; - switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - // Memory allocation failures are exposed in copyErrorTo() - if (other.fPtr.dfs != nullptr) { - fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); - } else { - fPtr.dfs = nullptr; - } - break; - case SYMPTR_NS: - // Memory allocation failures are exposed in copyErrorTo() - if (other.fPtr.ns != nullptr) { - fPtr.ns = new NumberingSystem(*other.fPtr.ns); - } else { - fPtr.ns = nullptr; - } - break; - } -} - -void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { - fType = src.fType; - switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - fPtr.dfs = src.fPtr.dfs; - src.fPtr.dfs = nullptr; - break; - case SYMPTR_NS: - fPtr.ns = src.fPtr.ns; - src.fPtr.ns = nullptr; - break; - } -} - -// void SymbolsWrapper::doCleanup() { -// switch (fType) { -// case SYMPTR_NONE: -// // No action necessary -// break; -// case SYMPTR_DFS: -// delete fPtr.dfs; -// break; -// case SYMPTR_NS: -// delete fPtr.ns; -// break; -// } -// } - -bool SymbolsWrapper::isDecimalFormatSymbols() const { - return fType == SYMPTR_DFS; -} - -bool SymbolsWrapper::isNumberingSystem() const { - return fType == SYMPTR_NS; -} - -const DecimalFormatSymbols* SymbolsWrapper::getDecimalFormatSymbols() const { - U_ASSERT(fType == SYMPTR_DFS); - return fPtr.dfs; -} - -const NumberingSystem* SymbolsWrapper::getNumberingSystem() const { - U_ASSERT(fType == SYMPTR_NS); - return fPtr.ns; -} - - FormattedNumber LocalizedNumberFormatter::formatInt(int64_t value, UErrorCode& status) const { if (U_FAILURE(status)) { return FormattedNumber(U_ILLEGAL_ARGUMENT_ERROR); } auto results = new UFormattedNumberData(); diff --git a/icu4c/source/i18n/number_symbolswrapper.cpp b/icu4c/source/i18n/number_symbolswrapper.cpp index 9a819721500..063ed3994c5 100644 --- a/icu4c/source/i18n/number_symbolswrapper.cpp +++ b/icu4c/source/i18n/number_symbolswrapper.cpp @@ -8,91 +8,89 @@ using namespace icu; using namespace icu::number; using namespace icu::number::impl; -MicroPropsGenerator::~MicroPropsGenerator() = default; +SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { + doCopyFrom(other); +} -// SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { -// doCopyFrom(other); -// } +SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { + doMoveFrom(std::move(src)); +} -// SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { -// doMoveFrom(std::move(src)); -// } +SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { + if (this == &other) { + return *this; + } + doCleanup(); + doCopyFrom(other); + return *this; +} -// SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { -// if (this == &other) { -// return *this; -// } -// doCleanup(); -// doCopyFrom(other); -// return *this; -// } - -// SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { -// if (this == &src) { -// return *this; -// } -// doCleanup(); -// doMoveFrom(std::move(src)); -// return *this; -// } +SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { + if (this == &src) { + return *this; + } + doCleanup(); + doMoveFrom(std::move(src)); + return *this; +} SymbolsWrapper::~SymbolsWrapper() { doCleanup(); } -// void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { -// doCleanup(); -// fType = SYMPTR_DFS; -// fPtr.dfs = new DecimalFormatSymbols(dfs); -// } +void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { + doCleanup(); + fType = SYMPTR_DFS; + fPtr.dfs = new DecimalFormatSymbols(dfs); +} -// void SymbolsWrapper::setTo(const NumberingSystem* ns) { -// doCleanup(); -// fType = SYMPTR_NS; -// fPtr.ns = ns; -// } +void SymbolsWrapper::setTo(const NumberingSystem* ns) { + doCleanup(); + fType = SYMPTR_NS; + fPtr.ns = ns; +} -// void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { -// fType = other.fType; -// switch (fType) { -// case SYMPTR_NONE: -// // No action necessary -// break; -// case SYMPTR_DFS: -// // Memory allocation failures are exposed in copyErrorTo() -// if (other.fPtr.dfs != nullptr) { -// fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); -// } else { -// fPtr.dfs = nullptr; -// } -// break; -// case SYMPTR_NS: -// // Memory allocation failures are exposed in copyErrorTo() -// if (other.fPtr.ns != nullptr) { -// fPtr.ns = new NumberingSystem(*other.fPtr.ns); -// } else { -// fPtr.ns = nullptr; -// } -// break; -// } -// } +void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { + fType = other.fType; + switch (fType) { + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + // Memory allocation failures are exposed in copyErrorTo() + if (other.fPtr.dfs != nullptr) { + fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); + } else { + fPtr.dfs = nullptr; + } + break; + case SYMPTR_NS: + // Memory allocation failures are exposed in copyErrorTo() + if (other.fPtr.ns != nullptr) { + fPtr.ns = new NumberingSystem(*other.fPtr.ns); + } else { + fPtr.ns = nullptr; + } + break; + } +} -// void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { -// fType = src.fType; -// switch (fType) { -// case SYMPTR_NONE: -// // No action necessary -// break; -// case SYMPTR_DFS: -// fPtr.dfs = src.fPtr.dfs; -// src.fPtr.dfs = nullptr; -// break; -// case SYMPTR_NS: -// fPtr.ns = src.fPtr.ns; -// src.fPtr.ns = nullptr; -// break; -// } -// } +void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { + fType = src.fType; + switch (fType) { + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + fPtr.dfs = src.fPtr.dfs; + src.fPtr.dfs = nullptr; + break; + case SYMPTR_NS: + fPtr.ns = src.fPtr.ns; + src.fPtr.ns = nullptr; + break; + } +} void SymbolsWrapper::doCleanup() { switch (fType) { @@ -108,20 +106,20 @@ void SymbolsWrapper::doCleanup() { } } -// bool SymbolsWrapper::isDecimalFormatSymbols() const { -// return fType == SYMPTR_DFS; -// } +bool SymbolsWrapper::isDecimalFormatSymbols() const { + return fType == SYMPTR_DFS; +} -// bool SymbolsWrapper::isNumberingSystem() const { -// return fType == SYMPTR_NS; -// } +bool SymbolsWrapper::isNumberingSystem() const { + return fType == SYMPTR_NS; +} -// const DecimalFormatSymbols* SymbolsWrapper::getDecimalFormatSymbols() const { -// U_ASSERT(fType == SYMPTR_DFS); -// return fPtr.dfs; -// } +const DecimalFormatSymbols* SymbolsWrapper::getDecimalFormatSymbols() const { + U_ASSERT(fType == SYMPTR_DFS); + return fPtr.dfs; +} -// const NumberingSystem* SymbolsWrapper::getNumberingSystem() const { -// U_ASSERT(fType == SYMPTR_NS); -// return fPtr.ns; -// } +const NumberingSystem* SymbolsWrapper::getNumberingSystem() const { + U_ASSERT(fType == SYMPTR_NS); + return fPtr.ns; +} diff --git a/icu4c/source/i18n/number_types.h b/icu4c/source/i18n/number_types.h index 8180fe55317..8078851ba3f 100644 --- a/icu4c/source/i18n/number_types.h +++ b/icu4c/source/i18n/number_types.h @@ -262,7 +262,7 @@ class U_I18N_API ModifierStore { */ class U_I18N_API MicroPropsGenerator { public: - virtual ~MicroPropsGenerator(); + virtual ~MicroPropsGenerator() = default; /** * Considers the given {@link DecimalQuantity}, optionally mutates it, and diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index d0d8c3ba342..668fad01d65 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -994,15 +994,24 @@ group: numberformatter number_notation.o number_padding.o number_patternmodifier.o number_patternstring.o number_scientific.o - currpinf.o dcfmtsym.o numsys.o + currpinf.o numrange_fluent.o numrange_impl.o deps decnumber double_conversion formattable units unitsformatter number_representation number_output + numsys numberformatter_microprops uclean_i18n common number_symbolswrapper +group: numsys + dcfmtsym.o + numsys.o + deps + currency + resourcebundle + uclean_i18n + group: numberformatter_microprops number_multiplier.o number_usageprefs.o @@ -1028,6 +1037,7 @@ group: number_symbolswrapper number_symbolswrapper.o deps platform + numsys group: numberparser numparse_affixes.o numparse_compositions.o numparse_currency.o From b4833af6c831e93881376cdda9d489b1fbf19e58 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 04:13:19 +0200 Subject: [PATCH 11/25] While we've moved code, might as well clang-format it. --- icu4c/source/i18n/number_rounding.cpp | 4 +- icu4c/source/i18n/number_symbolswrapper.cpp | 98 ++++++++++----------- icu4c/source/i18n/number_usageprefs.cpp | 2 +- icu4c/source/i18n/unicode/numberformatter.h | 11 +-- 4 files changed, 58 insertions(+), 57 deletions(-) diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index f76d0d16cc5..aa830403abc 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -24,8 +24,8 @@ using namespace icu::number::impl; using double_conversion::DoubleToStringConverter; using icu::StringSegment; -void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, - UErrorCode& status) { +void blueprint_helpers::parseIncrementOption(const StringSegment &segment, MacroProps ¯os, + UErrorCode &status) { // Need to do char <-> UChar conversion... U_ASSERT(U_SUCCESS(status)); CharString buffer; diff --git a/icu4c/source/i18n/number_symbolswrapper.cpp b/icu4c/source/i18n/number_symbolswrapper.cpp index 063ed3994c5..5f7648d7039 100644 --- a/icu4c/source/i18n/number_symbolswrapper.cpp +++ b/icu4c/source/i18n/number_symbolswrapper.cpp @@ -8,15 +8,15 @@ using namespace icu; using namespace icu::number; using namespace icu::number::impl; -SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper& other) { +SymbolsWrapper::SymbolsWrapper(const SymbolsWrapper &other) { doCopyFrom(other); } -SymbolsWrapper::SymbolsWrapper(SymbolsWrapper&& src) U_NOEXCEPT { +SymbolsWrapper::SymbolsWrapper(SymbolsWrapper &&src) U_NOEXCEPT { doMoveFrom(std::move(src)); } -SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { +SymbolsWrapper &SymbolsWrapper::operator=(const SymbolsWrapper &other) { if (this == &other) { return *this; } @@ -25,7 +25,7 @@ SymbolsWrapper& SymbolsWrapper::operator=(const SymbolsWrapper& other) { return *this; } -SymbolsWrapper& SymbolsWrapper::operator=(SymbolsWrapper&& src) U_NOEXCEPT { +SymbolsWrapper &SymbolsWrapper::operator=(SymbolsWrapper &&src) U_NOEXCEPT { if (this == &src) { return *this; } @@ -38,71 +38,71 @@ SymbolsWrapper::~SymbolsWrapper() { doCleanup(); } -void SymbolsWrapper::setTo(const DecimalFormatSymbols& dfs) { +void SymbolsWrapper::setTo(const DecimalFormatSymbols &dfs) { doCleanup(); fType = SYMPTR_DFS; fPtr.dfs = new DecimalFormatSymbols(dfs); } -void SymbolsWrapper::setTo(const NumberingSystem* ns) { +void SymbolsWrapper::setTo(const NumberingSystem *ns) { doCleanup(); fType = SYMPTR_NS; fPtr.ns = ns; } -void SymbolsWrapper::doCopyFrom(const SymbolsWrapper& other) { +void SymbolsWrapper::doCopyFrom(const SymbolsWrapper &other) { fType = other.fType; switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - // Memory allocation failures are exposed in copyErrorTo() - if (other.fPtr.dfs != nullptr) { - fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); - } else { - fPtr.dfs = nullptr; - } - break; - case SYMPTR_NS: - // Memory allocation failures are exposed in copyErrorTo() - if (other.fPtr.ns != nullptr) { - fPtr.ns = new NumberingSystem(*other.fPtr.ns); - } else { - fPtr.ns = nullptr; - } - break; + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + // Memory allocation failures are exposed in copyErrorTo() + if (other.fPtr.dfs != nullptr) { + fPtr.dfs = new DecimalFormatSymbols(*other.fPtr.dfs); + } else { + fPtr.dfs = nullptr; + } + break; + case SYMPTR_NS: + // Memory allocation failures are exposed in copyErrorTo() + if (other.fPtr.ns != nullptr) { + fPtr.ns = new NumberingSystem(*other.fPtr.ns); + } else { + fPtr.ns = nullptr; + } + break; } } -void SymbolsWrapper::doMoveFrom(SymbolsWrapper&& src) { +void SymbolsWrapper::doMoveFrom(SymbolsWrapper &&src) { fType = src.fType; switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - fPtr.dfs = src.fPtr.dfs; - src.fPtr.dfs = nullptr; - break; - case SYMPTR_NS: - fPtr.ns = src.fPtr.ns; - src.fPtr.ns = nullptr; - break; + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + fPtr.dfs = src.fPtr.dfs; + src.fPtr.dfs = nullptr; + break; + case SYMPTR_NS: + fPtr.ns = src.fPtr.ns; + src.fPtr.ns = nullptr; + break; } } void SymbolsWrapper::doCleanup() { switch (fType) { - case SYMPTR_NONE: - // No action necessary - break; - case SYMPTR_DFS: - delete fPtr.dfs; - break; - case SYMPTR_NS: - delete fPtr.ns; - break; + case SYMPTR_NONE: + // No action necessary + break; + case SYMPTR_DFS: + delete fPtr.dfs; + break; + case SYMPTR_NS: + delete fPtr.ns; + break; } } @@ -114,12 +114,12 @@ bool SymbolsWrapper::isNumberingSystem() const { return fType == SYMPTR_NS; } -const DecimalFormatSymbols* SymbolsWrapper::getDecimalFormatSymbols() const { +const DecimalFormatSymbols *SymbolsWrapper::getDecimalFormatSymbols() const { U_ASSERT(fType == SYMPTR_DFS); return fPtr.dfs; } -const NumberingSystem* SymbolsWrapper::getNumberingSystem() const { +const NumberingSystem *SymbolsWrapper::getNumberingSystem() const { U_ASSERT(fType == SYMPTR_NS); return fPtr.ns; } diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 8ccdac8ea27..faa1dae514e 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -31,7 +31,7 @@ Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorC status = U_INVALID_FORMAT_ERROR; return {}; } - U_ASSERT(precisionSkeleton[kSkelPrefixLen-1] == u'/'); + U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); StringSegment segment(precisionSkeleton, false); segment.adjustOffset(kSkelPrefixLen); MacroProps macros; diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index b6b786a70cf..c6bfd551392 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -717,9 +717,12 @@ class U_I18N_API Precision : public UMemory { Precision(UErrorCode errorCode) : fType(RND_ERROR) { fUnion.errorCode = errorCode; } -public: // Constructor needed by parseSkeletonToPrecision: - Precision() : fType(RND_BOGUS) {} -private: + + public: // Constructor needed by parseSkeletonToPrecision: + Precision() : fType(RND_BOGUS) { + } + + private: bool isBogus() const { return fType == RND_BOGUS; } @@ -768,8 +771,6 @@ private: // To allow access to the skeleton generation code: friend class impl::GeneratorHelpers; - - // friend class impl::UsagePrefsHandler; }; /** From 051b31713e6dec5c0d70102a109fe555e274fd51 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 04:16:51 +0200 Subject: [PATCH 12/25] Add Precision::bogus() factory to keep Precision() private. --- icu4c/source/i18n/number_usageprefs.cpp | 4 ++-- icu4c/source/i18n/unicode/numberformatter.h | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index faa1dae514e..ae9c6164211 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -24,12 +24,12 @@ using icu::StringSegment; Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status) { if (U_FAILURE(status)) { - return {}; + return Precision::bogus(); } constexpr int32_t kSkelPrefixLen = 20; if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { status = U_INVALID_FORMAT_ERROR; - return {}; + return Precision::bogus(); } U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); StringSegment segment(precisionSkeleton, false); diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index c6bfd551392..821fdaa4bff 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -654,6 +654,13 @@ class U_I18N_API Precision : public UMemory { */ static CurrencyPrecision currency(UCurrencyUsage currencyUsage); +#ifndef U_HIDE_INTERNAL_API + /** Returns a bogus Precision instance. */ + static Precision bogus() { + return {}; + } +#endif /* U_HIDE_INTERNAL_API */ + private: enum PrecisionType { RND_BOGUS, @@ -718,11 +725,9 @@ class U_I18N_API Precision : public UMemory { fUnion.errorCode = errorCode; } - public: // Constructor needed by parseSkeletonToPrecision: Precision() : fType(RND_BOGUS) { } - private: bool isBogus() const { return fType == RND_BOGUS; } From bd41bca957f91284577c4ce080f48adcd74607cd Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 07:25:53 +0200 Subject: [PATCH 13/25] Rename new group to 'number_usageprefs', now that the dust has settled. --- icu4c/source/test/depstest/dependencies.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 668fad01d65..92dc82f1d7d 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -870,7 +870,7 @@ library: i18n listformatter formatting formattable_cnv regex regex_cnv translit double_conversion number_representation number_output numberformatter - number_skeletons numberformatter_microprops numberparser + number_skeletons number_usageprefs numberparser units_extra unitsformatter universal_time_scale uclean_i18n @@ -1000,7 +1000,7 @@ group: numberformatter decnumber double_conversion formattable units unitsformatter number_representation number_output numsys - numberformatter_microprops + number_usageprefs uclean_i18n common number_symbolswrapper @@ -1012,7 +1012,7 @@ group: numsys resourcebundle uclean_i18n -group: numberformatter_microprops +group: number_usageprefs number_multiplier.o number_usageprefs.o deps From 6d320dbefeb75d4f1c9587e385f54c7867cf6631 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 21 Aug 2020 16:32:47 +0200 Subject: [PATCH 14/25] Move "default rounding" bool into private Precision member --- icu4c/source/i18n/number_formatimpl.cpp | 11 ++----- icu4c/source/i18n/number_roundingutils.h | 6 ++++ icu4c/source/i18n/number_usageprefs.cpp | 35 ++++++++++----------- icu4c/source/i18n/unicode/numberformatter.h | 20 ++++++++++++ 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index 98281508278..047fdb0ad6a 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -246,7 +246,6 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, // Rounding strategy Precision precision; - bool defaultRounding = false; if (!macros.precision.isBogus()) { precision = macros.precision; } else if (macros.notation.fType == Notation::NTN_COMPACT) { @@ -254,8 +253,8 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, } else if (isCurrency) { precision = Precision::currency(UCURR_USAGE_STANDARD); } else { - defaultRounding = true; precision = Precision::maxFraction(6); + precision.setDefault(); } UNumberFormatRoundingMode roundingMode; if (macros.roundingMode != kDefaultMode) { @@ -264,13 +263,7 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, // Temporary until ICU 64 roundingMode = precision.fRoundingMode; } - if (defaultRounding && macros.usage.isSet()) { - // If defaultRounding and usage is set, we lave the rounding up to - // UsagePrefsHandler. - U_ASSERT(fUsagePrefsHandler.getAlias() != nullptr); - } else { - fMicros.rounder = {precision, roundingMode, currency, status}; - } + fMicros.rounder = {precision, roundingMode, currency, status}; if (U_FAILURE(status)) { return nullptr; } diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index 4207ab274b5..b9e05684846 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -189,6 +189,12 @@ class RoundingImpl { /** Version of {@link #apply} that obeys minInt constraints. Used for scientific notation compatibility mode. */ void apply(impl::DecimalQuantity &value, int32_t minInt, UErrorCode status); + /** + * If true, this RoundingImpl can be considered a default. Presently this + * just depends on whether fPrecision represents a default. + */ + bool isDefault() { return fPrecision.isDefault(); } + private: Precision fPrecision; UNumberFormatRoundingMode fRoundingMode; diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index ae9c6164211..3aaa59fcc2b 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -129,25 +129,22 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m UnicodeString precisionSkeleton = routed.precision; // TODO: Is it okay if this is kDefaultMode? // Otherwise it was: unitPrefMacros.roundingMode or precision.fRoundingMode; - UNumberFormatRoundingMode roundingMode = kDefaultMode; - CurrencyUnit currency(u"", status); - if (!micros.rounder.isPassThrough()) { - // Do nothing: we already have a rounder, so we don't use - // precisionSkeleton or a default "usage-appropriate" rounder. - } else if (precisionSkeleton.length() > 0) { - CharString csPrecisionSkeleton; - UErrorCode csErrCode = U_ZERO_ERROR; - csPrecisionSkeleton.appendInvariantChars(precisionSkeleton, csErrCode); - - // Parse skeleton, collect results - // int32_t errOffset; - // int32_t errOffset = 0; - U_ASSERT(U_SUCCESS(status)); - Precision precision = parseSkeletonToPrecision(precisionSkeleton, status); - micros.rounder = {precision, roundingMode, currency, status}; - } else { - Precision precision = Precision::integer().withMinDigits(2); - micros.rounder = {precision, roundingMode, currency, status}; + if (micros.rounder.isDefault()) { + UNumberFormatRoundingMode roundingMode = kDefaultMode; // TODO: appropriate? + CurrencyUnit currency(u"", status); + if (precisionSkeleton.length() > 0) { + // Parse skeleton, collect results + Precision precision = parseSkeletonToPrecision(precisionSkeleton, status); + micros.rounder = {precision, roundingMode, currency, status}; + } else { + // TODO: some disgreement as to whether to do this override. The default + // is maxFraction(6), which I find inappropriate for human-friendly + // usage-based unit formatting? We should probably specify a "default + // expectation when skeleton isn't given in unitPreferences", primarily + // so we don't have to add that to every preference. + Precision precision = Precision::integer().withMinDigits(2); + micros.rounder = {precision, roundingMode, currency, status}; + } } } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 821fdaa4bff..6256eee3bd1 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -654,6 +654,20 @@ class U_I18N_API Precision : public UMemory { */ static CurrencyPrecision currency(UCurrencyUsage currencyUsage); + /** + * If true, this Precision instance represents a default, so client code may + * more freely override it. + */ + bool isDefault() const { return fPrecisionIsDefault; } + + /** + * Sets the default flag: client code may freely override this Precision + * instance. + */ + void setDefault() { + fPrecisionIsDefault = true; + } + #ifndef U_HIDE_INTERNAL_API /** Returns a bogus Precision instance. */ static Precision bogus() { @@ -717,6 +731,12 @@ class U_I18N_API Precision : public UMemory { /** The Precision encapsulates the RoundingMode when used within the implementation. */ UNumberFormatRoundingMode fRoundingMode; + /** + * If true, this Precision instance is just a default, so client code can + * more freely override it. + */ + bool fPrecisionIsDefault = false; + Precision(const PrecisionType& type, const PrecisionUnion& union_, UNumberFormatRoundingMode roundingMode) : fType(type), fUnion(union_), fRoundingMode(roundingMode) {} From c6ad33d35bbfc15fa43f007a97daa5214759871c Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 22 Aug 2020 09:46:31 +0200 Subject: [PATCH 15/25] Use assertFormatSingle: it is neat and to the point. --- icu4c/source/test/intltest/numbertest_api.cpp | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 54eb954ae05..eef2d39b225 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -793,18 +793,16 @@ void NumberFormatterApiTest::unitUsageErrorCodes() { void NumberFormatterApiTest::unitUsageSkeletons() { IcuTestErrorCode status(*this, "unitUsageSkeletons()"); - // Default >300m road preference skeletons round to 50m - auto roadFormatter = NumberFormatter::forSkeleton("usage/road unit/meter", status).locale("en-ZA"); - auto result = roadFormatter.formatDouble(321, status).toString(status); - status.assertSuccess(); - assertEquals("unitUsageSkeletons() usage/road unit/meter en-ZA", "300 m", result); + assertFormatSingle(u"Default >300m road preference skeletons round to 50m", // + u"usage/road measure-unit/length-meter", u"usage/road unit/meter", // + NumberFormatter::with().unit(METER).usage("road"), // + Locale("en-ZA"), 321, u"300 m"); - // Override the roadFormatter's precision - roadFormatter = roadFormatter.precision(Precision::maxSignificantDigits(2)); - result = roadFormatter.formatDouble(321, status).toString(status); - status.assertSuccess(); - assertEquals("unitUsageSkeletons() precision override (usage/road unit/meter en-ZA)", "320 m", - result); + assertFormatSingle( + u"Precision can be overridden: override takes precedence", // + u"usage/road measure-unit/length-meter @#", u"usage/road unit/meter @#", + NumberFormatter::with().unit(METER).usage("road").precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), 321, u"320 m"); } void NumberFormatterApiTest::unitCompoundMeasure() { From d9ce728f6c9cb64b84b667482bb92e47c060d9f3 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 22 Aug 2020 10:00:38 +0200 Subject: [PATCH 16/25] Add scientific and compact notation test cases. WIP: a bug in scientific formatting. --- icu4c/source/test/intltest/numbertest_api.cpp | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index eef2d39b225..63c41c924b1 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -803,6 +803,65 @@ void NumberFormatterApiTest::unitUsageSkeletons() { u"usage/road measure-unit/length-meter @#", u"usage/road unit/meter @#", NumberFormatter::with().unit(METER).usage("road").precision(Precision::maxSignificantDigits(2)), Locale("en-ZA"), 321, u"320 m"); + + assertFormatSingle(u"Compact notation with Usage: bizarre, but possible (short)", + u"compact-short usage/road measure-unit/length-meter", + u"compact-short usage/road unit/meter", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactShort()), + Locale("en-ZA"), 987654321, u"988K km"); + + assertFormatSingle(u"Compact notation with Usage: bizarre, but possible (short, precision override)", + u"compact-short usage/road measure-unit/length-meter @#", + u"compact-short usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactShort()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), 987654321, u"990K km"); + + assertFormatSingle(u"Compact notation with Usage: unusual but possible (long)", + u"compact-long usage/road measure-unit/length-meter @#", + u"compact-long usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactLong()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), 987654321, u"990 thousand km"); + + assertFormatSingle(u"Compact notation with Usage: unusual but possible (long, precision override)", + u"compact-long usage/road measure-unit/length-meter @#", + u"compact-long usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactLong()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), 987654321, u"990 thousand km"); + +// // Bad test case: we don't want 0E2 as output +// assertFormatSingle(u"Scientific notation with Usage: unusual but possible", // +// u"scientific usage/road measure-unit/length-meter", +// u"scientific usage/road unit/meter", +// NumberFormatter::with() +// .unit(METER) +// .usage("road") +// .notation(Notation::scientific()), +// Locale("en-ZA"), 321.45, u"0E2 m"); + + assertFormatSingle(u"Scientific notation with Usage: unusual but possible (precision override)", + u"scientific usage/road measure-unit/length-meter @###", + u"scientific usage/road unit/meter @###", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::scientific()) + .precision(Precision::maxSignificantDigits(4)), + Locale("en-ZA"), 321.45, u"3,215E2 m"); } void NumberFormatterApiTest::unitCompoundMeasure() { From d85dad9b19566b8a2887e6111e15137bba3f028c Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 22 Aug 2020 15:04:18 +0200 Subject: [PATCH 17/25] Made sense of scientific notation. WAI, so fix unit testing examples. --- icu4c/source/test/intltest/numbertest_api.cpp | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 63c41c924b1..6267871189a 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -843,17 +843,14 @@ void NumberFormatterApiTest::unitUsageSkeletons() { .precision(Precision::maxSignificantDigits(2)), Locale("en-ZA"), 987654321, u"990 thousand km"); -// // Bad test case: we don't want 0E2 as output -// assertFormatSingle(u"Scientific notation with Usage: unusual but possible", // -// u"scientific usage/road measure-unit/length-meter", -// u"scientific usage/road unit/meter", -// NumberFormatter::with() -// .unit(METER) -// .usage("road") -// .notation(Notation::scientific()), -// Locale("en-ZA"), 321.45, u"0E2 m"); + assertFormatSingle( + u"Scientific notation, not recommended, requires precision override for road", + u"scientific usage/road measure-unit/length-meter", u"scientific usage/road unit/meter", + NumberFormatter::with().unit(METER).usage("road").notation(Notation::scientific()), + // Rounding to the nearest "50" is not exponent-adjusted in scientific notation: + Locale("en-ZA"), 321.45, u"0E2 m"); - assertFormatSingle(u"Scientific notation with Usage: unusual but possible (precision override)", + assertFormatSingle(u"Scientific notation with Usage: possible when using a reasonable Precision", u"scientific usage/road measure-unit/length-meter @###", u"scientific usage/road unit/meter @###", NumberFormatter::with() @@ -862,6 +859,18 @@ void NumberFormatterApiTest::unitUsageSkeletons() { .notation(Notation::scientific()) .precision(Precision::maxSignificantDigits(4)), Locale("en-ZA"), 321.45, u"3,215E2 m"); + + assertFormatSingle( + u"Scientific notation with Usage: possible when using a reasonable Precision", + u"scientific usage/default measure-unit/length-astronomical-unit unit-width-full-name", + u"scientific usage/default unit/astronomical-unit unit-width-full-name", + NumberFormatter::with() + .unit(MeasureUnit::forIdentifier("astronomical-unit", status)) + .usage("default") + .notation(Notation::scientific()) + .unitWidth(UNumberUnitWidth::UNUM_UNIT_WIDTH_FULL_NAME), + Locale("en-ZA"), 1e20, u"1,5E28 kilometres"); + status.assertSuccess(); } void NumberFormatterApiTest::unitCompoundMeasure() { From 893851d9dad7867c54b520638e6157f4403a2124 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 22 Aug 2020 19:38:14 +0200 Subject: [PATCH 18/25] Add a scientific & square-meter example to unitUsage(). --- icu4c/source/test/intltest/numbertest_api.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 6267871189a..64bea85289f 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -767,6 +767,26 @@ void NumberFormatterApiTest::unitUsage() { u"30 ft", u"0 ft"); + assertFormatDescendingBig( + u"Scientific notation with Usage: possible when using a reasonable Precision", + u"scientific @### usage/default measure-unit/area-square-meter unit-width-full-name", + u"scientific @### usage/default unit/square-meter unit-width-full-name", + NumberFormatter::with() + .unit(SQUARE_METER) + .usage("default") + .notation(Notation::scientific()) + .precision(Precision::minMaxSignificantDigits(1, 4)) + .unitWidth(UNumberUnitWidth::UNUM_UNIT_WIDTH_FULL_NAME), + Locale("en-ZA"), + u"8,765E1 square kilometres", + u"8,765E0 square kilometres", + u"8,765E1 hectares", + u"8,765E0 hectares", + u"8,765E3 square metres", + u"8,765E2 square metres", + u"8,765E1 square metres", + u"8,765E0 square metres", + u"0E0 square centimetres"); } void NumberFormatterApiTest::unitUsageErrorCodes() { @@ -790,6 +810,8 @@ void NumberFormatterApiTest::unitUsageErrorCodes() { status.assertSuccess(); } +// Tests for the "skeletons" field in unitPreferenceData, as well as precision +// and notation overrides. void NumberFormatterApiTest::unitUsageSkeletons() { IcuTestErrorCode status(*this, "unitUsageSkeletons()"); From a065a05c3d20de96915e99a30519e61274731595 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 22 Aug 2020 20:19:24 +0200 Subject: [PATCH 19/25] UserPrefsHandler, as friend of RoundingImpl, now replaces only fPrecision. --- icu4c/source/i18n/number_formatimpl.cpp | 5 ++++- icu4c/source/i18n/number_roundingutils.h | 9 +++------ icu4c/source/i18n/number_usageprefs.cpp | 13 +++---------- icu4c/source/i18n/unicode/numberformatter.h | 2 -- 4 files changed, 10 insertions(+), 19 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index 047fdb0ad6a..2bf407f435a 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -252,9 +252,12 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, precision = Precision::integer().withMinDigits(2); } else if (isCurrency) { precision = Precision::currency(UCURR_USAGE_STANDARD); + } else if (macros.usage.isSet()) { + // Precision will get chosen in the UsagePrefsHandler + precision = Precision::bogus(); + precision.setDefault(); } else { precision = Precision::maxFraction(6); - precision.setDefault(); } UNumberFormatRoundingMode roundingMode; if (macros.roundingMode != kDefaultMode) { diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index b9e05684846..51223dd2d0b 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -189,16 +189,13 @@ class RoundingImpl { /** Version of {@link #apply} that obeys minInt constraints. Used for scientific notation compatibility mode. */ void apply(impl::DecimalQuantity &value, int32_t minInt, UErrorCode status); - /** - * If true, this RoundingImpl can be considered a default. Presently this - * just depends on whether fPrecision represents a default. - */ - bool isDefault() { return fPrecision.isDefault(); } - private: Precision fPrecision; UNumberFormatRoundingMode fRoundingMode; bool fPassThrough = true; // default value + + // Permits access to fPrecision. + friend class UsagePrefsHandler; }; diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 3aaa59fcc2b..67e1f8f857e 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -127,23 +127,16 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m quantity.setToDouble(routedUnits[0]->getNumber().getDouble()); UnicodeString precisionSkeleton = routed.precision; - // TODO: Is it okay if this is kDefaultMode? - // Otherwise it was: unitPrefMacros.roundingMode or precision.fRoundingMode; - if (micros.rounder.isDefault()) { - UNumberFormatRoundingMode roundingMode = kDefaultMode; // TODO: appropriate? - CurrencyUnit currency(u"", status); + if (micros.rounder.fPrecision.isDefault()) { if (precisionSkeleton.length() > 0) { - // Parse skeleton, collect results - Precision precision = parseSkeletonToPrecision(precisionSkeleton, status); - micros.rounder = {precision, roundingMode, currency, status}; + micros.rounder.fPrecision = parseSkeletonToPrecision(precisionSkeleton, status); } else { // TODO: some disgreement as to whether to do this override. The default // is maxFraction(6), which I find inappropriate for human-friendly // usage-based unit formatting? We should probably specify a "default // expectation when skeleton isn't given in unitPreferences", primarily // so we don't have to add that to every preference. - Precision precision = Precision::integer().withMinDigits(2); - micros.rounder = {precision, roundingMode, currency, status}; + micros.rounder.fPrecision = Precision::integer().withMinDigits(2); } } } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 6256eee3bd1..30c137b7775 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -774,8 +774,6 @@ class U_I18N_API Precision : public UMemory { static CurrencyPrecision constructCurrency(UCurrencyUsage usage); - static Precision constructPassThrough(); - // To allow MacroProps/MicroProps to initialize bogus instances: friend struct impl::MacroProps; friend struct impl::MicroProps; From 38e6e6e042aa448dedd92fa6893f89b0f7699fc2 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 12:13:14 +0200 Subject: [PATCH 20/25] Fix RoundingMode. FIXMEs annotating fRoundingMode use. --- icu4c/source/i18n/number_formatimpl.cpp | 8 ++++++++ icu4c/source/i18n/number_mapper.cpp | 6 ++++++ icu4c/source/i18n/number_rounding.cpp | 16 ++++++++-------- icu4c/source/i18n/number_usageprefs.cpp | 14 +++++++++----- icu4c/source/i18n/unicode/numberformatter.h | 15 ++++++++++++--- 5 files changed, 43 insertions(+), 16 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index 2bf407f435a..e84cfd0a488 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -261,8 +261,16 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, } UNumberFormatRoundingMode roundingMode; if (macros.roundingMode != kDefaultMode) { + // FIXME: we use macros.roundingMode if available. So as long as this is + // always available when precision.fRoundingMode is defined, + // precision.fRoundingMode is unneeded. roundingMode = macros.roundingMode; } else { + // FIXME: makes use of fRoundingMode to pick a default. This is the only + // actual usage of this value, so if we drop it here, we don't need it + // anymore. What was supposed to change in ICU 64? : + printf("macros.roundingMode was kDefaultMode (%d). precision.fRoundingMode is %d\n", + kDefaultMode, precision.fRoundingMode); // Temporary until ICU 64 roundingMode = precision.fRoundingMode; } diff --git a/icu4c/source/i18n/number_mapper.cpp b/icu4c/source/i18n/number_mapper.cpp index ec617438c9a..624c3e1b3d5 100644 --- a/icu4c/source/i18n/number_mapper.cpp +++ b/icu4c/source/i18n/number_mapper.cpp @@ -145,6 +145,9 @@ MacroProps NumberPropertyMapper::oldToNew(const DecimalFormatProperties& propert precision = Precision::constructCurrency(currencyUsage); } if (!precision.isBogus()) { + /* FIXME: setting precision fRoundingMode, we should also set + * macros.roundingMode for preferential use: */ + // macros.roundingMode = roundingMode; precision.fRoundingMode = roundingMode; macros.precision = precision; } @@ -239,6 +242,9 @@ MacroProps NumberPropertyMapper::oldToNew(const DecimalFormatProperties& propert // TODO: Reset maxSig_ = 1 + minFrac_ to follow the spec. macros.precision = Precision::constructSignificant(minSig_, maxSig_); } + /* FIXME: setting precision fRoundingMode, we should also set + * macros.roundingMode for preferential use: */ + // macros.roundingMode = roundingMode; macros.precision.fRoundingMode = roundingMode; } } diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index aa830403abc..e65843e25f2 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -119,7 +119,7 @@ digits_t roundingutils::doubleFractionLength(double input, int8_t* singleDigit) Precision Precision::unlimited() { - return Precision(RND_NONE, {}, kDefaultMode); + return Precision(RND_NONE, {}); } FractionPrecision Precision::integer() { @@ -264,7 +264,7 @@ FractionPrecision Precision::constructFraction(int32_t minFrac, int32_t maxFrac) settings.fMaxSig = -1; PrecisionUnion union_; union_.fracSig = settings; - return {RND_FRACTION, union_, kDefaultMode}; + return {RND_FRACTION, union_}; } Precision Precision::constructSignificant(int32_t minSig, int32_t maxSig) { @@ -275,7 +275,7 @@ Precision Precision::constructSignificant(int32_t minSig, int32_t maxSig) { settings.fMaxSig = static_cast(maxSig); PrecisionUnion union_; union_.fracSig = settings; - return {RND_SIGNIFICANT, union_, kDefaultMode}; + return {RND_SIGNIFICANT, union_}; } Precision @@ -285,7 +285,7 @@ Precision::constructFractionSignificant(const FractionPrecision &base, int32_t m settings.fMaxSig = static_cast(maxSig); PrecisionUnion union_; union_.fracSig = settings; - return {RND_FRACTION_SIGNIFICANT, union_, kDefaultMode}; + return {RND_FRACTION_SIGNIFICANT, union_}; } IncrementPrecision Precision::constructIncrement(double increment, int32_t minFrac) { @@ -305,18 +305,18 @@ IncrementPrecision Precision::constructIncrement(double increment, int32_t minFr // NOTE: In C++, we must return the correct value type with the correct union. // It would be invalid to return a RND_FRACTION here because the methods on the // IncrementPrecision type assume that the union is backed by increment data. - return {RND_INCREMENT_ONE, union_, kDefaultMode}; + return {RND_INCREMENT_ONE, union_}; } else if (singleDigit == 5) { - return {RND_INCREMENT_FIVE, union_, kDefaultMode}; + return {RND_INCREMENT_FIVE, union_}; } else { - return {RND_INCREMENT, union_, kDefaultMode}; + return {RND_INCREMENT, union_}; } } CurrencyPrecision Precision::constructCurrency(UCurrencyUsage usage) { PrecisionUnion union_; union_.currencyUsage = usage; - return {RND_CURRENCY, union_, kDefaultMode}; + return {RND_CURRENCY, union_}; } diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 67e1f8f857e..b4a84c68ff1 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -130,13 +130,17 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m if (micros.rounder.fPrecision.isDefault()) { if (precisionSkeleton.length() > 0) { micros.rounder.fPrecision = parseSkeletonToPrecision(precisionSkeleton, status); + // FIXME: overriding fRoundingMode, it lacks a default. + printf("fRoundingMode: %d\n", micros.rounder.fRoundingMode); + micros.rounder.fRoundingMode = kDefaultMode; } else { - // TODO: some disgreement as to whether to do this override. The default - // is maxFraction(6), which I find inappropriate for human-friendly - // usage-based unit formatting? We should probably specify a "default - // expectation when skeleton isn't given in unitPreferences", primarily - // so we don't have to add that to every preference. + // We use the same rounding mode as COMPACT notation: known to be a + // human-friendly rounding mode: integers, but add a decimal digit + // as needed to ensure we have at least 2 significant digits. micros.rounder.fPrecision = Precision::integer().withMinDigits(2); + // FIXME: overriding fRoundingMode, it lacks a default. + printf("fRoundingMode: %d\n", micros.rounder.fRoundingMode); + micros.rounder.fRoundingMode = kDefaultMode; } } } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 30c137b7775..1bc44fd1f6e 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -10,6 +10,7 @@ #if !UCONFIG_NO_FORMATTING +#include "unum.h" // FIXME: We don't want this here. Temporary access to kDefaultMode's value. #include "unicode/appendable.h" #include "unicode/bytestream.h" #include "unicode/currunit.h" @@ -728,6 +729,10 @@ class U_I18N_API Precision : public UMemory { typedef PrecisionUnion::FractionSignificantSettings FractionSignificantSettings; typedef PrecisionUnion::IncrementSettings IncrementSettings; + // FIXME: this FIXME commit is for determining where fRoundingMode is used, + // so we can get rid of it. It could have been defaulted to kRoundingMode + // right here, but we're investigating constructors for now (and think it's + // entirely unneeded). /** The Precision encapsulates the RoundingMode when used within the implementation. */ UNumberFormatRoundingMode fRoundingMode; @@ -737,9 +742,13 @@ class U_I18N_API Precision : public UMemory { */ bool fPrecisionIsDefault = false; - Precision(const PrecisionType& type, const PrecisionUnion& union_, - UNumberFormatRoundingMode roundingMode) - : fType(type), fUnion(union_), fRoundingMode(roundingMode) {} + // FIXME: Initializes fRoundingMode. At all call-sites for this constructor, + // this was simply set to kDefaultMode. In this FIXME commit, I'm + // de-parameterising this, until we decide we don't need it at all. + // kDefaultMode=UNUM_ROUND_HALFEVEN. (Had trouble referencing kDefaultMode + // for some reason.) + Precision(const PrecisionType& type, const PrecisionUnion& union_) + : fType(type), fUnion(union_), fRoundingMode(UNUM_ROUND_HALFEVEN) {} Precision(UErrorCode errorCode) : fType(RND_ERROR) { fUnion.errorCode = errorCode; From 3cd5702f719dd640262f86d5509356870a03f36d Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 12:24:05 +0200 Subject: [PATCH 21/25] Drop Precision::fRoundingMode entirely. --- icu4c/source/i18n/number_formatimpl.cpp | 15 +-------------- icu4c/source/i18n/number_mapper.cpp | 10 ++-------- icu4c/source/i18n/number_usageprefs.cpp | 6 ------ icu4c/source/i18n/unicode/numberformatter.h | 15 +-------------- 4 files changed, 4 insertions(+), 42 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index e84cfd0a488..f44d7a1ee59 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -260,20 +260,7 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, precision = Precision::maxFraction(6); } UNumberFormatRoundingMode roundingMode; - if (macros.roundingMode != kDefaultMode) { - // FIXME: we use macros.roundingMode if available. So as long as this is - // always available when precision.fRoundingMode is defined, - // precision.fRoundingMode is unneeded. - roundingMode = macros.roundingMode; - } else { - // FIXME: makes use of fRoundingMode to pick a default. This is the only - // actual usage of this value, so if we drop it here, we don't need it - // anymore. What was supposed to change in ICU 64? : - printf("macros.roundingMode was kDefaultMode (%d). precision.fRoundingMode is %d\n", - kDefaultMode, precision.fRoundingMode); - // Temporary until ICU 64 - roundingMode = precision.fRoundingMode; - } + roundingMode = macros.roundingMode; fMicros.rounder = {precision, roundingMode, currency, status}; if (U_FAILURE(status)) { return nullptr; diff --git a/icu4c/source/i18n/number_mapper.cpp b/icu4c/source/i18n/number_mapper.cpp index 624c3e1b3d5..29bcc54cc54 100644 --- a/icu4c/source/i18n/number_mapper.cpp +++ b/icu4c/source/i18n/number_mapper.cpp @@ -145,10 +145,7 @@ MacroProps NumberPropertyMapper::oldToNew(const DecimalFormatProperties& propert precision = Precision::constructCurrency(currencyUsage); } if (!precision.isBogus()) { - /* FIXME: setting precision fRoundingMode, we should also set - * macros.roundingMode for preferential use: */ - // macros.roundingMode = roundingMode; - precision.fRoundingMode = roundingMode; + macros.roundingMode = roundingMode; macros.precision = precision; } @@ -242,10 +239,7 @@ MacroProps NumberPropertyMapper::oldToNew(const DecimalFormatProperties& propert // TODO: Reset maxSig_ = 1 + minFrac_ to follow the spec. macros.precision = Precision::constructSignificant(minSig_, maxSig_); } - /* FIXME: setting precision fRoundingMode, we should also set - * macros.roundingMode for preferential use: */ - // macros.roundingMode = roundingMode; - macros.precision.fRoundingMode = roundingMode; + macros.roundingMode = roundingMode; } } diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index b4a84c68ff1..edaa0edd706 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -130,17 +130,11 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m if (micros.rounder.fPrecision.isDefault()) { if (precisionSkeleton.length() > 0) { micros.rounder.fPrecision = parseSkeletonToPrecision(precisionSkeleton, status); - // FIXME: overriding fRoundingMode, it lacks a default. - printf("fRoundingMode: %d\n", micros.rounder.fRoundingMode); - micros.rounder.fRoundingMode = kDefaultMode; } else { // We use the same rounding mode as COMPACT notation: known to be a // human-friendly rounding mode: integers, but add a decimal digit // as needed to ensure we have at least 2 significant digits. micros.rounder.fPrecision = Precision::integer().withMinDigits(2); - // FIXME: overriding fRoundingMode, it lacks a default. - printf("fRoundingMode: %d\n", micros.rounder.fRoundingMode); - micros.rounder.fRoundingMode = kDefaultMode; } } } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 1bc44fd1f6e..45148667de9 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -10,7 +10,6 @@ #if !UCONFIG_NO_FORMATTING -#include "unum.h" // FIXME: We don't want this here. Temporary access to kDefaultMode's value. #include "unicode/appendable.h" #include "unicode/bytestream.h" #include "unicode/currunit.h" @@ -729,26 +728,14 @@ class U_I18N_API Precision : public UMemory { typedef PrecisionUnion::FractionSignificantSettings FractionSignificantSettings; typedef PrecisionUnion::IncrementSettings IncrementSettings; - // FIXME: this FIXME commit is for determining where fRoundingMode is used, - // so we can get rid of it. It could have been defaulted to kRoundingMode - // right here, but we're investigating constructors for now (and think it's - // entirely unneeded). - /** The Precision encapsulates the RoundingMode when used within the implementation. */ - UNumberFormatRoundingMode fRoundingMode; - /** * If true, this Precision instance is just a default, so client code can * more freely override it. */ bool fPrecisionIsDefault = false; - // FIXME: Initializes fRoundingMode. At all call-sites for this constructor, - // this was simply set to kDefaultMode. In this FIXME commit, I'm - // de-parameterising this, until we decide we don't need it at all. - // kDefaultMode=UNUM_ROUND_HALFEVEN. (Had trouble referencing kDefaultMode - // for some reason.) Precision(const PrecisionType& type, const PrecisionUnion& union_) - : fType(type), fUnion(union_), fRoundingMode(UNUM_ROUND_HALFEVEN) {} + : fType(type), fUnion(union_) {} Precision(UErrorCode errorCode) : fType(RND_ERROR) { fUnion.errorCode = errorCode; From 12efd77d645fc549abe38b02581e207cd657c9a5 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 12:53:26 +0200 Subject: [PATCH 22/25] Cleanup: undo unnecessary changes. Shuffle code around for friendship. --- icu4c/source/i18n/number_formatimpl.cpp | 5 +-- icu4c/source/i18n/number_rounding.cpp | 2 + icu4c/source/i18n/number_roundingutils.h | 3 -- icu4c/source/i18n/number_skeletons.cpp | 45 +++------------------ icu4c/source/i18n/number_usageprefs.cpp | 10 +++-- icu4c/source/i18n/number_usageprefs.h | 4 ++ icu4c/source/i18n/unicode/numberformatter.h | 33 ++------------- icu4c/source/test/depstest/dependencies.txt | 2 +- 8 files changed, 25 insertions(+), 79 deletions(-) diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp index f44d7a1ee59..3f2a00f7e7c 100644 --- a/icu4c/source/i18n/number_formatimpl.cpp +++ b/icu4c/source/i18n/number_formatimpl.cpp @@ -253,9 +253,8 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe, } else if (isCurrency) { precision = Precision::currency(UCURR_USAGE_STANDARD); } else if (macros.usage.isSet()) { - // Precision will get chosen in the UsagePrefsHandler - precision = Precision::bogus(); - precision.setDefault(); + // Bogus Precision - it will get set in the UsagePrefsHandler instead + precision = Precision(); } else { precision = Precision::maxFraction(6); } diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index e65843e25f2..14896679696 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -24,6 +24,8 @@ using namespace icu::number::impl; using double_conversion::DoubleToStringConverter; using icu::StringSegment; +// Most blueprint_helpers live in number_skeletons.cpp. This one is in +// number_rounding.cpp for dependency reasons. void blueprint_helpers::parseIncrementOption(const StringSegment &segment, MacroProps ¯os, UErrorCode &status) { // Need to do char <-> UChar conversion... diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index 51223dd2d0b..841854fab54 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -163,9 +163,6 @@ class RoundingImpl { /** Required for ScientificFormatter */ bool isSignificantDigits() const; - /** Returns true if this is a default do-nothing instance */ - bool isPassThrough() const { return fPassThrough; } - /** * Rounding endpoint used by Engineering and Compact notation. Chooses the most appropriate multiplier (magnitude * adjustment), applies the adjustment, rounds, and returns the chosen multiplier. diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index 1f5ec47049f..704a5045b78 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -432,6 +432,11 @@ UnlocalizedNumberFormatter skeleton::create( perror->postContext[0] = 0; } + umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); + if (U_FAILURE(status)) { + return {}; + } + int32_t errOffset; MacroProps macros = parseSkeleton(skeletonString, errOffset, status); if (U_SUCCESS(status)) { @@ -462,14 +467,7 @@ UnicodeString skeleton::generate(const MacroProps& macros, UErrorCode& status) { MacroProps skeleton::parseSkeleton( const UnicodeString& skeletonString, int32_t& errOffset, UErrorCode& status) { - umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); - - // parseStem and parseOption expect U_SUCCESS(status). We check status here - // and we return as soon as U_FAILURE(status) becomes true. - if (U_FAILURE(status)) { - return MacroProps(); - } - + U_ASSERT(U_SUCCESS(status)); U_ASSERT(kSerializedStemTrie != nullptr); // Add a trailing whitespace to the end of the skeleton string to make code cleaner. @@ -1330,37 +1328,6 @@ bool blueprint_helpers::parseFracSigOption(const StringSegment& segment, MacroPr return true; } -// void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, -// UErrorCode& status) { -// // Need to do char <-> UChar conversion... -// U_ASSERT(U_SUCCESS(status)); -// CharString buffer; -// SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); - -// // Utilize DecimalQuantity/decNumber to parse this for us. -// DecimalQuantity dq; -// UErrorCode localStatus = U_ZERO_ERROR; -// dq.setToDecNumber({buffer.data(), buffer.length()}, localStatus); -// if (U_FAILURE(localStatus)) { -// // throw new SkeletonSyntaxException("Invalid rounding increment", segment, e); -// status = U_NUMBER_SKELETON_SYNTAX_ERROR; -// return; -// } -// double increment = dq.toDouble(); - -// // We also need to figure out how many digits. Do a brute force string operation. -// int decimalOffset = 0; -// while (decimalOffset < segment.length() && segment.charAt(decimalOffset) != '.') { -// decimalOffset++; -// } -// if (decimalOffset == segment.length()) { -// macros.precision = Precision::increment(increment); -// } else { -// int32_t fractionLength = segment.length() - decimalOffset - 1; -// macros.precision = Precision::increment(increment).withMinFraction(fractionLength); -// } -// } - void blueprint_helpers::generateIncrementOption(double increment, int32_t trailingZeros, UnicodeString& sb, UErrorCode&) { // Utilize DecimalQuantity/double_conversion to format this for us. diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index edaa0edd706..af2dfb35c14 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -22,14 +22,16 @@ using namespace icu::number; using namespace icu::number::impl; using icu::StringSegment; -Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status) { +// TODO: Move down. +Precision UsagePrefsHandler::parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, + UErrorCode status) { if (U_FAILURE(status)) { - return Precision::bogus(); + return {}; } constexpr int32_t kSkelPrefixLen = 20; if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { status = U_INVALID_FORMAT_ERROR; - return Precision::bogus(); + return {}; } U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); StringSegment segment(precisionSkeleton, false); @@ -127,7 +129,7 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m quantity.setToDouble(routedUnits[0]->getNumber().getDouble()); UnicodeString precisionSkeleton = routed.precision; - if (micros.rounder.fPrecision.isDefault()) { + if (micros.rounder.fPrecision.isBogus()) { if (precisionSkeleton.length() > 0) { micros.rounder.fPrecision = parseSkeletonToPrecision(precisionSkeleton, status); } else { diff --git a/icu4c/source/i18n/number_usageprefs.h b/icu4c/source/i18n/number_usageprefs.h index fa56c1ea5c7..7d0538101d7 100644 --- a/icu4c/source/i18n/number_usageprefs.h +++ b/icu4c/source/i18n/number_usageprefs.h @@ -52,6 +52,10 @@ class U_I18N_API UsagePrefsHandler : public MicroPropsGenerator, public UMemory private: UnitsRouter fUnitsRouter; const MicroPropsGenerator *fParent; + + // As a friend class, parseSkeletonToPrecision has access to Precision's + // default constructor (producing bogus instances). + static Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status); }; } // namespace impl diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 45148667de9..b2c22bb135f 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -654,27 +654,6 @@ class U_I18N_API Precision : public UMemory { */ static CurrencyPrecision currency(UCurrencyUsage currencyUsage); - /** - * If true, this Precision instance represents a default, so client code may - * more freely override it. - */ - bool isDefault() const { return fPrecisionIsDefault; } - - /** - * Sets the default flag: client code may freely override this Precision - * instance. - */ - void setDefault() { - fPrecisionIsDefault = true; - } - -#ifndef U_HIDE_INTERNAL_API - /** Returns a bogus Precision instance. */ - static Precision bogus() { - return {}; - } -#endif /* U_HIDE_INTERNAL_API */ - private: enum PrecisionType { RND_BOGUS, @@ -728,12 +707,6 @@ class U_I18N_API Precision : public UMemory { typedef PrecisionUnion::FractionSignificantSettings FractionSignificantSettings; typedef PrecisionUnion::IncrementSettings IncrementSettings; - /** - * If true, this Precision instance is just a default, so client code can - * more freely override it. - */ - bool fPrecisionIsDefault = false; - Precision(const PrecisionType& type, const PrecisionUnion& union_) : fType(type), fUnion(union_) {} @@ -741,8 +714,7 @@ class U_I18N_API Precision : public UMemory { fUnion.errorCode = errorCode; } - Precision() : fType(RND_BOGUS) { - } + Precision() : fType(RND_BOGUS) {} bool isBogus() const { return fType == RND_BOGUS; @@ -790,6 +762,9 @@ class U_I18N_API Precision : public UMemory { // To allow access to the skeleton generation code: friend class impl::GeneratorHelpers; + + // To allow access to isBogus and the default (bogus) constructor: + friend class impl::UsagePrefsHandler; }; /** diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 92dc82f1d7d..b43ceadd280 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -1013,7 +1013,7 @@ group: numsys uclean_i18n group: number_usageprefs - number_multiplier.o + number_multiplier.o number_usageprefs.o deps number_rounding From e57d4a93f889939c4cf5202508c2d9cf81c1d5e0 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 12:59:34 +0200 Subject: [PATCH 23/25] Reorder functions in number_usageprefs.cpp for consistency. --- icu4c/source/i18n/number_usageprefs.cpp | 39 +++++++++++++------------ icu4c/source/i18n/number_usageprefs.h | 2 -- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index af2dfb35c14..4738bfa5b0b 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -22,25 +22,6 @@ using namespace icu::number; using namespace icu::number::impl; using icu::StringSegment; -// TODO: Move down. -Precision UsagePrefsHandler::parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, - UErrorCode status) { - if (U_FAILURE(status)) { - return {}; - } - constexpr int32_t kSkelPrefixLen = 20; - if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { - status = U_INVALID_FORMAT_ERROR; - return {}; - } - U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); - StringSegment segment(precisionSkeleton, false); - segment.adjustOffset(kSkelPrefixLen); - MacroProps macros; - blueprint_helpers::parseIncrementOption(segment, macros, status); - return macros.precision; -} - // Copy constructor Usage::Usage(const Usage &other) : fUsage(nullptr), fLength(other.fLength), fError(other.fError) { if (other.fUsage != nullptr) { @@ -141,4 +122,24 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m } } +Precision UsagePrefsHandler::parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, + UErrorCode status) { + if (U_FAILURE(status)) { + // As a member of UsagePrefsHandler, which is a friend of Precision, we + // get access to the default constructor. + return {}; + } + constexpr int32_t kSkelPrefixLen = 20; + if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { + status = U_INVALID_FORMAT_ERROR; + return {}; + } + U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); + StringSegment segment(precisionSkeleton, false); + segment.adjustOffset(kSkelPrefixLen); + MacroProps macros; + blueprint_helpers::parseIncrementOption(segment, macros, status); + return macros.precision; +} + #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/i18n/number_usageprefs.h b/icu4c/source/i18n/number_usageprefs.h index 7d0538101d7..bdb2a54f82c 100644 --- a/icu4c/source/i18n/number_usageprefs.h +++ b/icu4c/source/i18n/number_usageprefs.h @@ -53,8 +53,6 @@ class U_I18N_API UsagePrefsHandler : public MicroPropsGenerator, public UMemory UnitsRouter fUnitsRouter; const MicroPropsGenerator *fParent; - // As a friend class, parseSkeletonToPrecision has access to Precision's - // default constructor (producing bogus instances). static Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status); }; From 9906ef6958bbb079b147ac4043231bf20ad63719 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 13:12:38 +0200 Subject: [PATCH 24/25] One more comment: clarifying roundingMode use in NumberPropertyMapper::oldToNew --- icu4c/source/i18n/number_mapper.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/icu4c/source/i18n/number_mapper.cpp b/icu4c/source/i18n/number_mapper.cpp index 29bcc54cc54..e2a0d284b7c 100644 --- a/icu4c/source/i18n/number_mapper.cpp +++ b/icu4c/source/i18n/number_mapper.cpp @@ -92,6 +92,8 @@ MacroProps NumberPropertyMapper::oldToNew(const DecimalFormatProperties& propert int32_t minSig = properties.minimumSignificantDigits; int32_t maxSig = properties.maximumSignificantDigits; double roundingIncrement = properties.roundingIncrement; + // Not assigning directly to macros.roundingMode here: we change + // roundingMode if and when we also change macros.precision. RoundingMode roundingMode = properties.roundingMode.getOrDefault(UNUM_ROUND_HALFEVEN); bool explicitMinMaxFrac = minFrac != -1 || maxFrac != -1; bool explicitMinMaxSig = minSig != -1 || maxSig != -1; From d2264474f4df8e6e82e4721fd4f3e98e03c275cc Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 26 Aug 2020 22:37:06 +0200 Subject: [PATCH 25/25] Code review feedback incorporated. --- icu4c/source/i18n/number_skeletons.cpp | 3 + icu4c/source/test/intltest/numbertest_api.cpp | 173 +++++++++++------- 2 files changed, 105 insertions(+), 71 deletions(-) diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index 704a5045b78..207fc05daac 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -1328,6 +1328,9 @@ bool blueprint_helpers::parseFracSigOption(const StringSegment& segment, MacroPr return true; } +// blueprint_helpers::parseIncrementOption lives in number_rounding.cpp for +// dependencies reasons. + void blueprint_helpers::generateIncrementOption(double increment, int32_t trailingZeros, UnicodeString& sb, UErrorCode&) { // Utilize DecimalQuantity/double_conversion to format this for us. diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 64bea85289f..0e4c976d401 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -815,83 +815,114 @@ void NumberFormatterApiTest::unitUsageErrorCodes() { void NumberFormatterApiTest::unitUsageSkeletons() { IcuTestErrorCode status(*this, "unitUsageSkeletons()"); - assertFormatSingle(u"Default >300m road preference skeletons round to 50m", // - u"usage/road measure-unit/length-meter", u"usage/road unit/meter", // - NumberFormatter::with().unit(METER).usage("road"), // - Locale("en-ZA"), 321, u"300 m"); + assertFormatSingle( + u"Default >300m road preference skeletons round to 50m", + u"usage/road measure-unit/length-meter", + u"usage/road unit/meter", + NumberFormatter::with().unit(METER).usage("road"), + Locale("en-ZA"), + 321, + u"300 m"); assertFormatSingle( - u"Precision can be overridden: override takes precedence", // - u"usage/road measure-unit/length-meter @#", u"usage/road unit/meter @#", - NumberFormatter::with().unit(METER).usage("road").precision(Precision::maxSignificantDigits(2)), - Locale("en-ZA"), 321, u"320 m"); - - assertFormatSingle(u"Compact notation with Usage: bizarre, but possible (short)", - u"compact-short usage/road measure-unit/length-meter", - u"compact-short usage/road unit/meter", - NumberFormatter::with() - .unit(METER) - .usage("road") - .notation(Notation::compactShort()), - Locale("en-ZA"), 987654321, u"988K km"); - - assertFormatSingle(u"Compact notation with Usage: bizarre, but possible (short, precision override)", - u"compact-short usage/road measure-unit/length-meter @#", - u"compact-short usage/road unit/meter @#", - NumberFormatter::with() - .unit(METER) - .usage("road") - .notation(Notation::compactShort()) - .precision(Precision::maxSignificantDigits(2)), - Locale("en-ZA"), 987654321, u"990K km"); - - assertFormatSingle(u"Compact notation with Usage: unusual but possible (long)", - u"compact-long usage/road measure-unit/length-meter @#", - u"compact-long usage/road unit/meter @#", - NumberFormatter::with() - .unit(METER) - .usage("road") - .notation(Notation::compactLong()) - .precision(Precision::maxSignificantDigits(2)), - Locale("en-ZA"), 987654321, u"990 thousand km"); - - assertFormatSingle(u"Compact notation with Usage: unusual but possible (long, precision override)", - u"compact-long usage/road measure-unit/length-meter @#", - u"compact-long usage/road unit/meter @#", - NumberFormatter::with() - .unit(METER) - .usage("road") - .notation(Notation::compactLong()) - .precision(Precision::maxSignificantDigits(2)), - Locale("en-ZA"), 987654321, u"990 thousand km"); + u"Precision can be overridden: override takes precedence", + u"usage/road measure-unit/length-meter @#", + u"usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), + 321, + u"320 m"); assertFormatSingle( - u"Scientific notation, not recommended, requires precision override for road", - u"scientific usage/road measure-unit/length-meter", u"scientific usage/road unit/meter", - NumberFormatter::with().unit(METER).usage("road").notation(Notation::scientific()), - // Rounding to the nearest "50" is not exponent-adjusted in scientific notation: - Locale("en-ZA"), 321.45, u"0E2 m"); - - assertFormatSingle(u"Scientific notation with Usage: possible when using a reasonable Precision", - u"scientific usage/road measure-unit/length-meter @###", - u"scientific usage/road unit/meter @###", - NumberFormatter::with() - .unit(METER) - .usage("road") - .notation(Notation::scientific()) - .precision(Precision::maxSignificantDigits(4)), - Locale("en-ZA"), 321.45, u"3,215E2 m"); + u"Compact notation with Usage: bizarre, but possible (short)", + u"compact-short usage/road measure-unit/length-meter", + u"compact-short usage/road unit/meter", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactShort()), + Locale("en-ZA"), + 987654321, + u"988K km"); assertFormatSingle( - u"Scientific notation with Usage: possible when using a reasonable Precision", - u"scientific usage/default measure-unit/length-astronomical-unit unit-width-full-name", - u"scientific usage/default unit/astronomical-unit unit-width-full-name", - NumberFormatter::with() - .unit(MeasureUnit::forIdentifier("astronomical-unit", status)) - .usage("default") - .notation(Notation::scientific()) - .unitWidth(UNumberUnitWidth::UNUM_UNIT_WIDTH_FULL_NAME), - Locale("en-ZA"), 1e20, u"1,5E28 kilometres"); + u"Compact notation with Usage: bizarre, but possible (short, precision override)", + u"compact-short usage/road measure-unit/length-meter @#", + u"compact-short usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactShort()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), + 987654321, + u"990K km"); + + assertFormatSingle( + u"Compact notation with Usage: unusual but possible (long)", + u"compact-long usage/road measure-unit/length-meter @#", + u"compact-long usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactLong()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), + 987654321, + u"990 thousand km"); + + assertFormatSingle( + u"Compact notation with Usage: unusual but possible (long, precision override)", + u"compact-long usage/road measure-unit/length-meter @#", + u"compact-long usage/road unit/meter @#", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::compactLong()) + .precision(Precision::maxSignificantDigits(2)), + Locale("en-ZA"), + 987654321, + u"990 thousand km"); + + assertFormatSingle( + u"Scientific notation, not recommended, requires precision override for road", + u"scientific usage/road measure-unit/length-meter", + u"scientific usage/road unit/meter", + NumberFormatter::with().unit(METER).usage("road").notation(Notation::scientific()), + Locale("en-ZA"), + 321.45, + // Rounding to the nearest "50" is not exponent-adjusted in scientific notation: + u"0E2 m"); + + assertFormatSingle( + u"Scientific notation with Usage: possible when using a reasonable Precision", + u"scientific usage/road measure-unit/length-meter @###", + u"scientific usage/road unit/meter @###", + NumberFormatter::with() + .unit(METER) + .usage("road") + .notation(Notation::scientific()) + .precision(Precision::maxSignificantDigits(4)), + Locale("en-ZA"), + 321.45, + u"3,215E2 m"); + + assertFormatSingle( + u"Scientific notation with Usage: possible when using a reasonable Precision", + u"scientific usage/default measure-unit/length-astronomical-unit unit-width-full-name", + u"scientific usage/default unit/astronomical-unit unit-width-full-name", + NumberFormatter::with() + .unit(MeasureUnit::forIdentifier("astronomical-unit", status)) + .usage("default") + .notation(Notation::scientific()) + .unitWidth(UNumberUnitWidth::UNUM_UNIT_WIDTH_FULL_NAME), + Locale("en-ZA"), + 1e20, + u"1,5E28 kilometres"); + status.assertSuccess(); }