Fixing test failures.

icu4c/source/common/cmemory.h

@@ -783,6 +783,19 @@ public:
     T* operator[](ptrdiff_t i) const {
         return this->fPool[i];
     }
+
+    /**
+     * Append all the items from another MaybeStackVector to this one.
+     */
+    void appendAll(const MaybeStackVector& other, UErrorCode& status) {
+        for (int32_t i = 0; i < other.fCount; i++) {
+            T* item = emplaceBack(*other[i]);
+            if (!item) {
+                status = U_MEMORY_ALLOCATION_ERROR;
+                return;
+            }
+        }
+    }
 };
 
 

icu4c/source/i18n/measunit.cpp

@@ -2041,7 +2041,7 @@ MeasureUnit &MeasureUnit::operator=(const MeasureUnit &other) {
     uprv_free(fImpl);
     if (other.fImpl) {
         ErrorCode localStatus;
-        fImpl = new MeasureUnitImpl(MeasureUnitImpl::forMeasureUnitMaybeCopy(*this, localStatus));
+        fImpl = new MeasureUnitImpl(other.fImpl->copy(localStatus));
         if (!fImpl || localStatus.isFailure()) {
             // Unrecoverable allocation error; set to the default unit
             *this = MeasureUnit();
@@ -2072,7 +2072,7 @@ MeasureUnit *MeasureUnit::clone() const {
 }
 
 MeasureUnit::~MeasureUnit() {
-    uprv_free(fImpl);
+    delete fImpl;
     fImpl = nullptr;
 }
 

icu4c/source/i18n/measunit_extra.cpp

@@ -530,7 +530,7 @@ void serializeSingle(const TempSingleUnit& singleUnit, bool first, CharString& o
     }
     int8_t posPower = std::abs(singleUnit.dimensionality);
     if (posPower == 0) {
-        status = U_ILLEGAL_ARGUMENT_ERROR;
+        status = U_INTERNAL_PROGRAM_ERROR;
     } else if (posPower == 1) {
         // no-op
     } else if (posPower == 2) {
@@ -621,7 +621,7 @@ bool appendImpl(MeasureUnitImpl& impl, const TempSingleUnit& unit, UErrorCode& s
     TempSingleUnit* oldUnit = nullptr;
     for (int32_t i = 0; i < impl.units.length(); i++) {
         auto* candidate = impl.units[i];
-        if (candidate->index == unit.index && candidate->siPrefix == unit.siPrefix) {
+        if (candidate->isCompatibleWith(unit)) {
             oldUnit = candidate;
         }
     }
@@ -680,25 +680,22 @@ const MeasureUnitImpl& MeasureUnitImpl::forMeasureUnit(
 
 MeasureUnitImpl MeasureUnitImpl::forMeasureUnitMaybeCopy(
         const MeasureUnit& measureUnit, UErrorCode& status) {
-    // TODO: Improve this algorithm to not round-trip through the identifier string?
-    return Parser::from(measureUnit.getIdentifier(), status).parse(status);
-}
-
-MeasureUnitImpl MeasureUnitImpl::forCurrencyCode(StringPiece currencyCode) {
-    MeasureUnitImpl result;
-    ErrorCode localStatus;
-    result.identifier.append(currencyCode, localStatus);
-    // localStatus is not expected to fail since currencyCode should be 3 chars long
-    return result;
+    if (measureUnit.fImpl) {
+        return measureUnit.fImpl->copy(status);
+    } else {
+        return Parser::from(measureUnit.getIdentifier(), status).parse(status);
+    }
 }
 
 void MeasureUnitImpl::takeReciprocal(UErrorCode& /*status*/) {
+    identifier.clear();
     for (int32_t i = 0; i < units.length(); i++) {
         units[i]->dimensionality *= -1;
     }
 }
 
 bool MeasureUnitImpl::append(const TempSingleUnit& singleUnit, UErrorCode& status) {
+    identifier.clear();
     return appendImpl(*this, singleUnit, status);
 }
 

icu4c/source/i18n/measunit_impl.h

@@ -45,6 +45,16 @@ struct TempSingleUnit : public UMemory {
         }
     }
 
+    /**
+     * Return whether this TempSingleUnit is compatible with another for the purpose of coalescing.
+     *
+     * Units with the same base unit and SI prefix should match, except that they must also have
+     * the same dimensionality sign, such that we don't merge numerator and denominator.
+     */
+    bool isCompatibleWith(const TempSingleUnit& other) const {
+        return (compareTo(other) == 0);
+    }
+
     /** Simple unit index, unique for every simple unit. */
     int32_t index = 0;
 
@@ -102,11 +112,28 @@ struct MeasureUnitImpl : public UMemory {
     /**
      * Used for currency units.
      */
-    static MeasureUnitImpl forCurrencyCode(StringPiece currencyCode);
+    static inline MeasureUnitImpl forCurrencyCode(StringPiece currencyCode) {
+        MeasureUnitImpl result;
+        UErrorCode localStatus = U_ZERO_ERROR;
+        result.identifier.append(currencyCode, localStatus);
+        // localStatus is not expected to fail since currencyCode should be 3 chars long
+        return result;
+    }
 
     /** Transform this MeasureUnitImpl into a MeasureUnit, simplifying if possible. */
     MeasureUnit build(UErrorCode& status) &&;
 
+    /**
+     * Create a copy of this MeasureUnitImpl. Don't use copy constructor to make this explicit.
+     */
+    inline MeasureUnitImpl copy(UErrorCode& status) const {
+        MeasureUnitImpl result;
+        result.complexity = complexity;
+        result.units.appendAll(units, status);
+        result.identifier.append(identifier, status);
+        return result;
+    }
+
     /** Mutates this MeasureUnitImpl to take the reciprocal. */
     void takeReciprocal(UErrorCode& status);
 

icu4c/source/test/depstest/dependencies.txt

@@ -1070,7 +1070,7 @@ group: units_extra
 group: units
     measunit.o currunit.o nounit.o
   deps
-    stringenumeration
+    stringenumeration errorcode
 
 group: decnumber
     decContext.o decNumber.o

icu4c/source/test/intltest/measfmttest.cpp

@@ -81,6 +81,8 @@ private:
     void TestNumericTimeSomeSpecialFormats();
     void TestInvalidIdentifiers();
     void TestCompoundUnitOperations();
+    void TestIdentifiers();
+
     void verifyFormat(
         const char *description,
         const MeasureFormat &fmt,
@@ -201,6 +203,7 @@ void MeasureFormatTest::runIndexedTest(
     TESTCASE_AUTO(TestNumericTimeSomeSpecialFormats);
     TESTCASE_AUTO(TestInvalidIdentifiers);
     TESTCASE_AUTO(TestCompoundUnitOperations);
+    TESTCASE_AUTO(TestIdentifiers);
     TESTCASE_AUTO_END;
 }
 
@@ -3400,6 +3403,29 @@ void MeasureFormatTest::TestCompoundUnitOperations() {
     assertTrue("kilometer equality", kilometer == kilometer2);
 }
 
+void MeasureFormatTest::TestIdentifiers() {
+    IcuTestErrorCode status(*this, "TestIdentifiers");
+    struct TestCase {
+        bool valid;
+        const char* id;
+        const char* normalized;
+    } cases[] = {
+        { true, "square-meter-per-square-meter", "square-meter-per-square-meter" },
+        // TODO(ICU-20920): Add more test cases once the proper ranking is available.
+    };
+    for (const auto& cas : cases) {
+        status.setScope(cas.id);
+        MeasureUnit unit = MeasureUnit::forIdentifier(cas.id, status);
+        if (!cas.valid) {
+            status.expectErrorAndReset(U_ILLEGAL_ARGUMENT_ERROR);
+            continue;
+        }
+        const char* actual = unit.getIdentifier();
+        assertEquals(cas.id, cas.normalized, actual);
+        status.errIfFailureAndReset();
+    }
+}
+
 
 void MeasureFormatTest::verifyFieldPosition(
         const char *description,