diff --git a/icu4c/source/common/cmemory.h b/icu4c/source/common/cmemory.h index 210bc7645e..a9d9424b4e 100644 --- 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 class MaybeStackVector : protected MemoryPool { public: - using MemoryPool::MemoryPool; - using MemoryPool::operator=; - template 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 deada9a9cd..7d5330f665 100644 --- 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 40bfc9d4c4..0f4f561731 100644 --- 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 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 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 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 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!