Improvements; TODO: getOutputUnit() still not implemented.

- For primitives: malloc/free, not new/delete.
- Some cleanup.
- Failing test cases for getOutputUnit(), with comments as to where it
  likely needs to be plumbed in.

diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp
index 2257b4793c10ad68fb0b44645b6af312924a8db1..f6145fa11f2ce7243c5294e46fb30e1630ea183b 100644 (file)
--- a/icu4c/source/i18n/number_fluent.cpp
+++ b/icu4c/source/i18n/number_fluent.cpp
@@ -282,7 +282,7 @@ Derived NumberFormatterSettings<Derived>::scale(const Scale& scale)&& {
 // Copy constructor
 Usage::Usage(const Usage &other) : fUsage(nullptr), fLength(other.fLength), fError(other.fError) {
     if (other.fUsage != nullptr) {
-        fUsage = new char[fLength + 1];
+        fUsage = (char*)uprv_malloc(fLength + 1);
         uprv_strncpy(fUsage, other.fUsage, fLength+1);
     }
 }
@@ -291,14 +291,17 @@ Usage::Usage(const Usage &other) : fUsage(nullptr), fLength(other.fLength), fErr
 Usage& Usage::operator=(const Usage& other) {
     fLength = other.fLength;
     if (other.fUsage != nullptr) {
-        fUsage = new char[fLength + 1];
+        fUsage = (char*)uprv_malloc(fLength + 1);
         uprv_strncpy(fUsage, other.fUsage, fLength+1);
     }
     fError = other.fError;
     return *this;
 }
 
-// Move constructor
+// Move constructor - can it be improved by taking over src's "this" instead of
+// copying contents? Swapping pointers makes sense for heap objects but not for
+// stack objects.
+// *this = std::move(src);
 Usage::Usage(Usage &&src) U_NOEXCEPT : fUsage(src.fUsage), fLength(src.fLength), fError(src.fError) {
     // Take ownership away from src if necessary
     src.fUsage = nullptr;
@@ -306,8 +309,11 @@ Usage::Usage(Usage &&src) U_NOEXCEPT : fUsage(src.fUsage), fLength(src.fLength),
 
 // Move assignment operator
 Usage& Usage::operator=(Usage&& src) U_NOEXCEPT {
+    if (this == &src) {
+        return *this;
+    }
     if (fUsage != nullptr) {
-        delete[] fUsage;
+        uprv_free(fUsage);
     }
     fUsage = src.fUsage;
     fLength = src.fLength;
@@ -319,7 +325,7 @@ Usage& Usage::operator=(Usage&& src) U_NOEXCEPT {
 
 Usage::~Usage() {
     if (fUsage != nullptr) {
-        delete[] fUsage;
+        uprv_free(fUsage);
         fUsage = nullptr;
     }
 }
@@ -327,11 +333,11 @@ Usage::~Usage() {
 void Usage::set(StringPiece value) {
     if (fUsage != nullptr) {
         // TODO: reuse if possible, rather than always delete?
-        delete[] fUsage;
+        uprv_free(fUsage);
         fUsage = nullptr;
     }
     fLength = value.length();
-    fUsage = new char[fLength+1];
+    fUsage = (char*)uprv_malloc(fLength+1);
     uprv_strncpy(fUsage, value.data(), fLength);
     fUsage[fLength] = 0;
 }
@@ -359,17 +365,6 @@ StubUnitsRouter::StubUnitsRouter(MeasureUnit inputUnit, StringPiece region,
     }
 }
 
-StubUnitsRouter::StubUnitsRouter(MeasureUnit inputUnit, Locale locale,
-                                 StringPiece usage, UErrorCode &status)
-    : fRegion(locale.getCountry(), status) {
-    if (usage.compare("road") != 0) {
-        status = U_UNSUPPORTED_ERROR;
-    }
-    if (inputUnit != MeasureUnit::getMeter()) {
-        status = U_UNSUPPORTED_ERROR;
-    }
-}
-
 MaybeStackVector<Measure> StubUnitsRouter::route(double quantity, UErrorCode &status) const {
     MeasureUnit unit;
     MaybeStackVector<Measure> result;
@@ -411,35 +406,6 @@ Derived NumberFormatterSettings<Derived>::usage(const StringPiece usage)&& {
     return move;
 }
 
-// WIP/FIXME/CLEANUP: I've implemented a "Units" class, for the fMacros.usage
-// member. Delete this comment block if that's good - initially I tried
-// `LocalArray<char> usage;`, which is what this comment contains.
-//
-// It turned out that to have a LocalArray field in MacroProps, LocalArray would
-// need a copy assignment operator), so I abandoned it:
-
-// template<typename Derived>
-// Derived NumberFormatterSettings<Derived>::usage(const StringPiece usage) const& {
-//     Derived copy(*this);
-//     int32_t usageLen = usage.length();
-//     char* newUsage = new char[usageLen+1];  // Do we need error handling for allocation failures?
-//     uprv_strncpy(newUsage, usage.data(), usageLen);
-//     newUsage[usageLen] = 0;
-//     copy.fMacros.usage.adoptInstead(newUsage);
-//     return copy;
-// }
-
-// template<typename Derived>
-// Derived NumberFormatterSettings<Derived>::usage(const StringPiece usage)&& {
-//     Derived move(std::move(*this));
-//     int32_t usageLen = usage.length();
-//     char* newUsage = new char[usageLen+1];  // Do we need error handling for allocation failures?
-//     uprv_strncpy(newUsage, usage.data(), usageLen);
-//     newUsage[usageLen] = 0;
-//     move.fMacros.usage.adoptInstead(newUsage);
-//     return move;
-// }
-
 template<typename Derived>
 Derived NumberFormatterSettings<Derived>::padding(const Padder& padder) const& {
     Derived copy(*this);
@@ -868,8 +834,14 @@ LocalizedNumberFormatter::formatDecimalQuantity(const DecimalQuantity& dq, UErro
 
 void LocalizedNumberFormatter::formatImpl(impl::UFormattedNumberData* results, UErrorCode& status) const {
     if (computeCompiled(status)) {
+        // FIXME: results needs outputUnit too, consider how to add that - pass
+        // `results` to formatStatic() instead of just results->quantity and
+        // ->getStringRef()? :
         fCompiled->format(results->quantity, results->getStringRef(), status);
     } else {
+        // FIXME: results needs outputUnit too, consider how to add that - pass
+        // `results` to formatStatic() instead of just results->quantity and
+        // ->getStringRef()? :
         NumberFormatterImpl::formatStatic(fMacros, results->quantity, results->getStringRef(), status);
     }
     if (U_FAILURE(status)) {

diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp
index 71da79bc96fffb847f32639d3d8abf47f8e52627..dc8307c5afacc21bbb8cf20d75927324dff43e40 100644 (file)
--- a/icu4c/source/i18n/number_formatimpl.cpp
+++ b/icu4c/source/i18n/number_formatimpl.cpp
@@ -362,21 +362,14 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe,
     // Outer modifier (CLDR units and currency long names)
     if (isCldrUnit) {
         if (macros.usage.isSet()) {
-            fLongNameMultiplexer.adoptInstead(
-                LongNameMultiplexer::forMeasureUnits(
-                    macros.locale,
-                    fUsagePrefsHandler->getOutputUnits(),
-                    unitWidth,
-                    resolvePluralRules(macros.rules, macros.locale, status),
-                    chain,
-                    status));
+            fLongNameMultiplexer.adoptInstead(LongNameMultiplexer::forMeasureUnits(
+                macros.locale, fUsagePrefsHandler->getOutputUnits(), unitWidth,
+                resolvePluralRules(macros.rules, macros.locale, status), chain, status));
             chain = fLongNameMultiplexer.getAlias();
         } else {
             fLongNameHandler.adoptInstead(LongNameHandler::forMeasureUnit(
-                macros.locale,
-                resolvedUnit,   // WIP/FIXME: not known at this time for usage()!
-                macros.perUnit, // WIP/FIXME: deal with COMPOUND and MIXED units?
-                unitWidth, resolvePluralRules(macros.rules, macros.locale, status), chain, status));
+                macros.locale, resolvedUnit, macros.perUnit, unitWidth,
+                resolvePluralRules(macros.rules, macros.locale, status), chain, status));
             chain = fLongNameHandler.getAlias();
         }
     } else if (isCurrency && unitWidth == UNUM_UNIT_WIDTH_FULL_NAME) {

diff --git a/icu4c/source/i18n/number_longnames.cpp b/icu4c/source/i18n/number_longnames.cpp
index 940281ef3c17b84f01f0d892d4f1be830dad7000..c3980581a6385a9cd3dcc912d58002fee1ac8579 100644 (file)
--- a/icu4c/source/i18n/number_longnames.cpp
+++ b/icu4c/source/i18n/number_longnames.cpp
@@ -376,12 +376,15 @@ LongNameMultiplexer::forMeasureUnits(const Locale &loc, const MaybeStackVector<M
     U_ASSERT(units.length() > 0);
     result->fMeasureUnits.adoptInstead(new MeasureUnit[units.length()]);
     for (int32_t i = 0, length = units.length(); i < length; i++) {
-        result->fLongNameHandlers.adoptBack(
-            LongNameHandler::forMeasureUnit(
-                loc, *units[i],
-                MeasureUnit(), // TODO(units): deal with COMPOUND and MIXED units
-                width, rules, NULL, status),
-            status);
+        auto *lnh = LongNameHandler::forMeasureUnit(
+            loc, *units[i],
+            MeasureUnit(), // TODO(units): deal with COMPOUND and MIXED units
+            width, rules, NULL, status);
+        if (U_FAILURE(status)) {
+            delete result;
+            return nullptr;
+        }
+        result->fLongNameHandlers.emplaceBack(std::move(*lnh));
         result->fMeasureUnits[i] = *units[i];
     }
     return result;

diff --git a/icu4c/source/i18n/number_longnames.h b/icu4c/source/i18n/number_longnames.h
index c029c7b54a947fdf8ced155df6719eb0bca25065..c0e60af1d33476fbeefcbe8a6f1b5a36056ec594 100644 (file)
--- a/icu4c/source/i18n/number_longnames.h
+++ b/icu4c/source/i18n/number_longnames.h
@@ -46,7 +46,9 @@ class LongNameHandler : public MicroPropsGenerator, public ModifierStore, public
 
   private:
     SimpleModifier fModifiers[StandardPlural::Form::COUNT];
+    // Not owned
     const PluralRules *rules;
+    // Not owned
     const MicroPropsGenerator *parent;
 
     LongNameHandler(const PluralRules *rules, const MicroPropsGenerator *parent)

diff --git a/icu4c/source/i18n/number_microprops.h b/icu4c/source/i18n/number_microprops.h
index 230130f941655eb5de0d42b66d36995ff67b7ef1..5dc35174900b1018488ef7417ec1729b3625a9e6 100644 (file)
--- a/icu4c/source/i18n/number_microprops.h
+++ b/icu4c/source/i18n/number_microprops.h
@@ -80,11 +80,14 @@ struct MicroProps : public MicroPropsGenerator {
      * this function can be used only once, because the base MicroProps instance
      * will be modified and thus not be available for re-use.
      *
+     * FIXME: document how the quantity passed in can be mutated by the chain of microprops' processQuantity methods.
+     *
      * @param quantity The quantity for consideration and optional mutation.
      * @param micros The MicroProps instance to populate. If this parameter is
      * not already `*this`, it will be overwritten with a copy of `*this`.
      */
-    void processQuantity(DecimalQuantity&, MicroProps& micros, UErrorCode& status) const U_OVERRIDE {
+    void processQuantity(DecimalQuantity &quantity, MicroProps &micros,
+                         UErrorCode &status) const U_OVERRIDE {
         (void) status;
         if (this == &micros) {
             // Unsafe path: no need to perform a copy.

diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index 9bb8fcb971c97899ce2991a8924a0f412aa2542e..1ac285fe51603aea3d1d2840bd8758b701a3c9ea 100644 (file)
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -410,7 +410,7 @@ void U_I18N_API UnitPreferences::getPreferencesFor(StringPiece category, StringP
     if (U_FAILURE(status)) { return; }
     U_ASSERT(idx >= 0); // Failures should have been taken care of by `status`.
     const UnitPreferenceMetadata *m = metadata_[idx];
-    outPreferences = unitPrefs_.getConstAlias() + m->prefsOffset;
+    outPreferences = unitPrefs_.getAlias() + m->prefsOffset;
     preferenceCount = m->prefsCount;
 }
 

diff --git a/icu4c/source/i18n/unitsrouter_stub.h b/icu4c/source/i18n/unitsrouter_stub.h
index 2fed92d5e356c30c8cbcff0c2b022629898a4c94..0380199e0ac63ecf6ff203f27a4a04d57c5a41c5 100644 (file)
--- a/icu4c/source/i18n/unitsrouter_stub.h
+++ b/icu4c/source/i18n/unitsrouter_stub.h
@@ -25,16 +25,12 @@
 
 U_NAMESPACE_BEGIN
 
-class U_I18N_API StubUnitsRouter {
+class U_I18N_API StubUnitsRouter : public UMemory {
   public:
     // As found in UnitsRouter
     StubUnitsRouter(MeasureUnit inputUnit, StringPiece region, StringPiece usage,
                     UErrorCode &status);
 
-    // TODO(units): consider this possible improvement for the constructor:
-    // passing "Locale" instead of "StringPiece region"? :
-    StubUnitsRouter(MeasureUnit inputUnit, Locale locale, StringPiece usage, UErrorCode &status);
-
     // TODO(units): API under reconsideration: going via Measure may be
     // excessive when we need a double; MaybeStackVector<Measure> might also not
     // be what we want for dealing with Mixed, given that a MeasureUnit can, on

diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt
index 8c28b7078394bd36950ff88aa17dc31ab0e1fa32..86854ee5eee82d73ae64e1adeccca0eb6f50daa7 100644 (file)
--- a/icu4c/source/test/depstest/dependencies.txt
+++ b/icu4c/source/test/depstest/dependencies.txt
@@ -979,6 +979,8 @@ group: number_output
     number_representation format formatted_value_sbimpl
     # PluralRules internals:
     unifiedcache
+    # Unit Formatting
+    units
 
 group: numberformatter
     # ICU 60+ NumberFormatter API

diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index 84ace5333bb786fd38e132ba96df833749829573..397e4d15d2d57e15b9ff94465a2b42d51dae9741 100644 (file)
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -683,15 +683,21 @@ void NumberFormatterApiTest::unitUsage() {
 
     LocalizedNumberFormatter formatter = unloc_formatter.locale("en-ZA");
     FormattedNumber formattedNum = formatter.formatDouble(300, status);
-    assertEquals("unitUsage() road", "300 m", formattedNum.toString(status));
+    assertTrue("unitUsage() en-ZA road outputUnit",
+               MeasureUnit::getMeter() == formattedNum.getOutputUnit(status));
+    assertEquals("unitUsage() en-ZA road", "300 m", formattedNum.toString(status));
 
     formatter = unloc_formatter.locale("en-GB");
     formattedNum = formatter.formatDouble(300, status);
-    assertEquals("unitUsage() road", "328 yd", formattedNum.toString(status));
+    assertTrue("unitUsage() en-GB road outputUnit",
+               MeasureUnit::getYard() == formattedNum.getOutputUnit(status));
+    assertEquals("unitUsage() en-GB road", "328 yd", formattedNum.toString(status));
 
     formatter = unloc_formatter.locale("en-US");
     formattedNum = formatter.formatDouble(300, status);
-    assertEquals("unitUsage() road", "984 ft", formattedNum.toString(status));
+    assertTrue("unitUsage() en-US road outputUnit",
+               MeasureUnit::getFoot() == formattedNum.getOutputUnit(status));
+    assertEquals("unitUsage() en-US road", "984 ft", formattedNum.toString(status));
 
     // TODO(hugovdm): consider fixing TODO(ICU-20941) too?
 }