ICU-21356 Fix memory handling in MemoryPool::operator=()

See #1437

diff --git a/icu4c/source/common/cmemory.h b/icu4c/source/common/cmemory.h
index 210bc7645e56e41ad6564eb45a46b05d6a90b751..a9d9424b4e247ec9a91e5ac13b6ac8a0d72972a7 100644 (file)
--- a/icu4c/source/common/cmemory.h
+++ b/icu4c/source/common/cmemory.h
@@ -725,9 +725,14 @@ public:
     }
 
     MemoryPool& operator=(MemoryPool&& other) U_NOEXCEPT {
-        fCount = other.fCount;
-        fPool = std::move(other.fPool);
-        other.fCount = 0;
+        // Since `this` may contain instances that need to be deleted, we can't
+        // just throw them away and replace them with `other`. The normal way of
+        // dealing with this in C++ is to swap `this` and `other`, rather than
+        // simply overwrite: the destruction of `other` can then take care of
+        // running MemoryPool::~MemoryPool() over the still-to-be-deallocated
+        // instances.
+        std::swap(fCount, other.fCount);
+        std::swap(fPool, other.fPool);
         return *this;
     }
 
@@ -796,9 +801,6 @@ protected:
 template<typename T, int32_t stackCapacity = 8>
 class MaybeStackVector : protected MemoryPool<T, stackCapacity> {
 public:
-    using MemoryPool<T, stackCapacity>::MemoryPool;
-    using MemoryPool<T, stackCapacity>::operator=;
-
     template<typename... Args>
     T* emplaceBack(Args&&... args) {
         return this->create(args...);

diff --git a/icu4c/source/test/intltest/measfmttest.cpp b/icu4c/source/test/intltest/measfmttest.cpp
index deada9a9cd66b6e13476cedd5fa7f6c9cd0b5f8b..7d5330f66571d66a38bb1874b2418fec31996d28 100644 (file)
--- a/icu4c/source/test/intltest/measfmttest.cpp
+++ b/icu4c/source/test/intltest/measfmttest.cpp
@@ -17,6 +17,9 @@
 
 #if !UCONFIG_NO_FORMATTING
 
+#include "charstr.h"
+#include "cstr.h"
+#include "measunit_impl.h"
 #include "unicode/decimfmt.h"
 #include "unicode/measfmt.h"
 #include "unicode/measure.h"
@@ -25,8 +28,6 @@
 #include "unicode/tmunit.h"
 #include "unicode/plurrule.h"
 #include "unicode/ustring.h"
-#include "charstr.h"
-#include "cstr.h"
 #include "unicode/reldatefmt.h"
 #include "unicode/rbnf.h"
 
@@ -88,6 +89,7 @@ private:
     void TestDimensionlessBehaviour();
     void Test21060_AddressSanitizerProblem();
     void Test21223_FrenchDuration();
+    void TestInternalMeasureUnitImpl();
 
     void verifyFormat(
         const char *description,
@@ -216,6 +218,7 @@ void MeasureFormatTest::runIndexedTest(
     TESTCASE_AUTO(TestDimensionlessBehaviour);
     TESTCASE_AUTO(Test21060_AddressSanitizerProblem);
     TESTCASE_AUTO(Test21223_FrenchDuration);
+    TESTCASE_AUTO(TestInternalMeasureUnitImpl);
     TESTCASE_AUTO_END;
 }
 
@@ -4037,6 +4040,56 @@ void MeasureFormatTest::Test21223_FrenchDuration() {
     // }
 }
 
+void MeasureFormatTest::TestInternalMeasureUnitImpl() {
+    IcuTestErrorCode status(*this, "TestInternalMeasureUnitImpl");
+    MeasureUnitImpl mu1 = MeasureUnitImpl::forIdentifier("meter", status);
+    status.assertSuccess();
+    assertEquals("mu1 initial identifier", "", mu1.identifier.data());
+    assertEquals("mu1 initial complexity", UMEASURE_UNIT_SINGLE, mu1.complexity);
+    assertEquals("mu1 initial units length", 1, mu1.units.length());
+    assertEquals("mu1 initial units[0]", "meter", mu1.units[0]->getSimpleUnitID());
+
+    // Producing identifier via build(): the std::move() means mu1 gets modified
+    // while it also gets assigned to tmp's internal fImpl.
+    MeasureUnit tmp = std::move(mu1).build(status);
+    status.assertSuccess();
+    assertEquals("mu1 post-move-build identifier", "meter", mu1.identifier.data());
+    assertEquals("mu1 post-move-build complexity", UMEASURE_UNIT_SINGLE, mu1.complexity);
+    assertEquals("mu1 post-move-build units length", 1, mu1.units.length());
+    assertEquals("mu1 post-move-build units[0]", "meter", mu1.units[0]->getSimpleUnitID());
+    assertEquals("MeasureUnit tmp identifier", "meter", tmp.getIdentifier());
+
+    // This temporary variable is used when forMeasureUnit's first parameter
+    // lacks an fImpl instance:
+    MeasureUnitImpl tmpMemory;
+    const MeasureUnitImpl &tmpImplRef = MeasureUnitImpl::forMeasureUnit(tmp, tmpMemory, status);
+    status.assertSuccess();
+    assertEquals("tmpMemory identifier", "", tmpMemory.identifier.data());
+    assertEquals("tmpMemory complexity", UMEASURE_UNIT_SINGLE, tmpMemory.complexity);
+    assertEquals("tmpMemory units length", 1, tmpMemory.units.length());
+    assertEquals("tmpMemory units[0]", "meter", tmpMemory.units[0]->getSimpleUnitID());
+    assertEquals("tmpImplRef identifier", "", tmpImplRef.identifier.data());
+    assertEquals("tmpImplRef complexity", UMEASURE_UNIT_SINGLE, tmpImplRef.complexity);
+
+    MeasureUnitImpl mu2 = MeasureUnitImpl::forIdentifier("newton-meter", status);
+    status.assertSuccess();
+    mu1 = std::move(mu2);
+    assertEquals("mu1 = move(mu2): identifier", "", mu1.identifier.data());
+    assertEquals("mu1 = move(mu2): complexity", UMEASURE_UNIT_COMPOUND, mu1.complexity);
+    assertEquals("mu1 = move(mu2): units length", 2, mu1.units.length());
+    assertEquals("mu1 = move(mu2): units[0]", "newton", mu1.units[0]->getSimpleUnitID());
+    assertEquals("mu1 = move(mu2): units[1]", "meter", mu1.units[1]->getSimpleUnitID());
+
+    mu1 = MeasureUnitImpl::forIdentifier("hour-and-minute-and-second", status);
+    status.assertSuccess();
+    assertEquals("mu1 = HMS: identifier", "", mu1.identifier.data());
+    assertEquals("mu1 = HMS: complexity", UMEASURE_UNIT_MIXED, mu1.complexity);
+    assertEquals("mu1 = HMS: units length", 3, mu1.units.length());
+    assertEquals("mu1 = HMS: units[0]", "hour", mu1.units[0]->getSimpleUnitID());
+    assertEquals("mu1 = HMS: units[1]", "minute", mu1.units[1]->getSimpleUnitID());
+    assertEquals("mu1 = HMS: units[2]", "second", mu1.units[2]->getSimpleUnitID());
+}
+
 
 void MeasureFormatTest::verifyFieldPosition(
         const char *description,

diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp
index 40bfc9d4c4076a0caa2e1c8023f2a932f5a59f31..0f4f561731d836dbb05c3ce2397cba54d11448b0 100644 (file)
--- a/icu4c/source/test/intltest/units_test.cpp
+++ b/icu4c/source/test/intltest/units_test.cpp
@@ -415,26 +415,25 @@ void UnitsTest::testComplexUnitsConverter() {
         assertEquals("1.9999: measures[0] value", 1.0, measures[0]->getNumber().getDouble(status));
         assertEquals("1.9999: measures[0] unit", MeasureUnit::getFoot().getIdentifier(),
                      measures[0]->getUnit().getIdentifier());
-        assertEqualsNear("1.9999: measures[1] value", 11.9988, measures[1]->getNumber().getDouble(status), 0.0001);
+        assertEqualsNear("1.9999: measures[1] value", 11.9988,
+                         measures[1]->getNumber().getDouble(status), 0.0001);
         assertEquals("1.9999: measures[1] unit", MeasureUnit::getInch().getIdentifier(),
                      measures[1]->getUnit().getIdentifier());
     }
 
     // TODO(icu-units#100): consider factoring out the set of tests to make this function more
-    // data-driven, *after* dealing appropriately with the memory leaks that can
-    // be demonstrated by this code.
+    // data-driven.
 
-    // TODO(icu-units#100): reusing measures results in a leak.
     // A minimal nudge under 2.0.
-    MaybeStackVector<Measure> measures2 = converter.convert((2.0 - DBL_EPSILON), nullptr, status);
-    assertEquals("measures length", 2, measures2.length());
-    if (2 == measures2.length()) {
-        assertEquals("1 - eps: measures[0] value", 2.0, measures2[0]->getNumber().getDouble(status));
+    measures = converter.convert((2.0 - DBL_EPSILON), nullptr, status);
+    assertEquals("measures length", 2, measures.length());
+    if (2 == measures.length()) {
+        assertEquals("1 - eps: measures[0] value", 2.0, measures[0]->getNumber().getDouble(status));
         assertEquals("1 - eps: measures[0] unit", MeasureUnit::getFoot().getIdentifier(),
-                     measures2[0]->getUnit().getIdentifier());
-        assertEquals("1 - eps: measures[1] value", 0.0, measures2[1]->getNumber().getDouble(status));
+                     measures[0]->getUnit().getIdentifier());
+        assertEquals("1 - eps: measures[1] value", 0.0, measures[1]->getNumber().getDouble(status));
         assertEquals("1 - eps: measures[1] unit", MeasureUnit::getInch().getIdentifier(),
-                     measures2[1]->getUnit().getIdentifier());
+                     measures[1]->getUnit().getIdentifier());
     }
 
     // Testing precision with meter and light-year. 1e-16 light years is
@@ -444,51 +443,51 @@ void UnitsTest::testComplexUnitsConverter() {
     // An epsilon's nudge under one light-year: should give 1 ly, 0 m.
     input = MeasureUnit::getLightYear();
     output = MeasureUnit::forIdentifier("light-year-and-meter", status);
-    // TODO(icu-units#100): reusing tempInput and tempOutput results in a leak.
-    MeasureUnitImpl tempInput3, tempOutput3;
-    const MeasureUnitImpl &inputImpl3 = MeasureUnitImpl::forMeasureUnit(input, tempInput3, status);
-    const MeasureUnitImpl &outputImpl3 = MeasureUnitImpl::forMeasureUnit(output, tempOutput3, status);
-    // TODO(icu-units#100): reusing converter results in a leak.
-    ComplexUnitsConverter converter3 = ComplexUnitsConverter(inputImpl3, outputImpl3, rates, status);
-    // TODO(icu-units#100): reusing measures results in a leak.
-    MaybeStackVector<Measure> measures3 = converter3.convert((2.0 - DBL_EPSILON), nullptr, status);
-    assertEquals("measures length", 2, measures3.length());
-    if (2 == measures3.length()) {
-        assertEquals("light-year test: measures[0] value", 2.0, measures3[0]->getNumber().getDouble(status));
+    const MeasureUnitImpl &inputImpl3 = MeasureUnitImpl::forMeasureUnit(input, tempInput, status);
+    const MeasureUnitImpl &outputImpl3 = MeasureUnitImpl::forMeasureUnit(output, tempOutput, status);
+    converter = ComplexUnitsConverter(inputImpl3, outputImpl3, rates, status);
+    measures = converter.convert((2.0 - DBL_EPSILON), nullptr, status);
+    assertEquals("measures length", 2, measures.length());
+    if (2 == measures.length()) {
+        assertEquals("light-year test: measures[0] value", 2.0,
+                     measures[0]->getNumber().getDouble(status));
         assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(),
-                     measures3[0]->getUnit().getIdentifier());
-        assertEquals("light-year test: measures[1] value", 0.0, measures3[1]->getNumber().getDouble(status));
+                     measures[0]->getUnit().getIdentifier());
+        assertEquals("light-year test: measures[1] value", 0.0,
+                     measures[1]->getNumber().getDouble(status));
         assertEquals("light-year test: measures[1] unit", MeasureUnit::getMeter().getIdentifier(),
-                     measures3[1]->getUnit().getIdentifier());
+                     measures[1]->getUnit().getIdentifier());
     }
 
     // 1e-15 light years is 9.46073 meters (calculated using "bc" and the CLDR
     // conversion factor). With double-precision maths, we get 10.5. In this
     // case, we're off by almost 1 meter.
-    MaybeStackVector<Measure> measures4 = converter3.convert((1.0 + 1e-15), nullptr, status);
-    assertEquals("measures length", 2, measures4.length());
-    if (2 == measures4.length()) {
-        assertEquals("light-year test: measures[0] value", 1.0, measures4[0]->getNumber().getDouble(status));
+    measures = converter.convert((1.0 + 1e-15), nullptr, status);
+    assertEquals("measures length", 2, measures.length());
+    if (2 == measures.length()) {
+        assertEquals("light-year test: measures[0] value", 1.0,
+                     measures[0]->getNumber().getDouble(status));
         assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(),
-                     measures4[0]->getUnit().getIdentifier());
+                     measures[0]->getUnit().getIdentifier());
         assertEqualsNear("light-year test: measures[1] value", 10,
-                         measures4[1]->getNumber().getDouble(status), 1);
+                         measures[1]->getNumber().getDouble(status), 1);
         assertEquals("light-year test: measures[1] unit", MeasureUnit::getMeter().getIdentifier(),
-                     measures4[1]->getUnit().getIdentifier());
+                     measures[1]->getUnit().getIdentifier());
     }
 
     // 2e-16 light years is 1.892146 meters. We consider this in the noise, and
     // thus expect a 0. (This test fails when 2e-16 is increased to 4e-16.)
-    MaybeStackVector<Measure> measures5 = converter3.convert((1.0 + 2e-16), nullptr, status);
-    assertEquals("measures length", 2, measures5.length());
-    if (2 == measures5.length()) {
-        assertEquals("light-year test: measures[0] value", 1.0, measures5[0]->getNumber().getDouble(status));
+    measures = converter.convert((1.0 + 2e-16), nullptr, status);
+    assertEquals("measures length", 2, measures.length());
+    if (2 == measures.length()) {
+        assertEquals("light-year test: measures[0] value", 1.0,
+                     measures[0]->getNumber().getDouble(status));
         assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(),
-                     measures5[0]->getUnit().getIdentifier());
+                     measures[0]->getUnit().getIdentifier());
         assertEquals("light-year test: measures[1] value", 0.0,
-                         measures5[1]->getNumber().getDouble(status));
+                     measures[1]->getNumber().getDouble(status));
         assertEquals("light-year test: measures[1] unit", MeasureUnit::getMeter().getIdentifier(),
-                     measures5[1]->getUnit().getIdentifier());
+                     measures[1]->getUnit().getIdentifier());
     }
 
     // TODO(icu-units#63): test negative numbers!