ICU-20050 Fixing memory leaks in move and copy assignment in Number*Formatter.

diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp
index e0e489a3573610d95716e009c641ea33cc8f8258..a66e3bd0f23510f610a975dacc7f69f8ab59ac4b 100644 (file)
--- a/icu4c/source/i18n/number_fluent.cpp
+++ b/icu4c/source/i18n/number_fluent.cpp
@@ -407,7 +407,8 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(NFS<LNF>&& src) U_NOEXCEPT
 
 LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(const LNF& other) {
     NFS<LNF>::operator=(static_cast<const NFS<LNF>&>(other));
-    // No additional fields to assign (let call count and compiled formatter reset to defaults)
+    // Reset to default values.
+    clear();
     return *this;
 }
 
@@ -419,20 +420,26 @@ LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(LNF&& src) U_NOEXC
         // Formatter is compiled
         lnfMoveHelper(static_cast<LNF&&>(src));
     } else {
-        // Reset to default values.
-        auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
-        umtx_storeRelease(*callCount, 0);
-        fCompiled = nullptr;
+        clear();
     }
     return *this;
 }
 
+void LocalizedNumberFormatter::clear() {
+    // Reset to default values.
+    auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
+    umtx_storeRelease(*callCount, 0);
+    delete fCompiled;
+    fCompiled = nullptr;
+}
+
 void LocalizedNumberFormatter::lnfMoveHelper(LNF&& src) {
     // Copy over the compiled formatter and set call count to INT32_MIN as in computeCompiled().
     // Don't copy the call count directly because doing so requires a loadAcquire/storeRelease.
     // The bits themselves appear to be platform-dependent, so copying them might not be safe.
     auto* callCount = reinterpret_cast<u_atomic_int32_t*>(fUnsafeCallCount);
     umtx_storeRelease(*callCount, INT32_MIN);
+    delete fCompiled;
     fCompiled = src.fCompiled;
     // Reset the source object to leave it in a safe state.
     auto* srcCallCount = reinterpret_cast<u_atomic_int32_t*>(src.fUnsafeCallCount);

diff --git a/icu4c/source/i18n/numrange_fluent.cpp b/icu4c/source/i18n/numrange_fluent.cpp
index aee9518fe09a92ea462205ced8705340b2fa02f7..4a812fd485e90693b92720e6a54aad0addff9af5 100644 (file)
--- a/icu4c/source/i18n/numrange_fluent.cpp
+++ b/icu4c/source/i18n/numrange_fluent.cpp
@@ -227,18 +227,26 @@ LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(LocalizedNumberRang
 
 LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(NFS<LNF>&& src) U_NOEXCEPT
         : NFS<LNF>(std::move(src)) {
-    // No additional fields to assign
+    // Steal the compiled formatter
+    LNF&& _src = static_cast<LNF&&>(src);
+    fImpl = _src.fImpl;
+    _src.fImpl = nullptr;
 }
 
 LocalizedNumberRangeFormatter& LocalizedNumberRangeFormatter::operator=(const LNF& other) {
     NFS<LNF>::operator=(static_cast<const NFS<LNF>&>(other));
-    // No additional fields to assign
+    // Do not steal; just clear
+    delete fImpl;
+    fImpl = nullptr;
     return *this;
 }
 
 LocalizedNumberRangeFormatter& LocalizedNumberRangeFormatter::operator=(LNF&& src) U_NOEXCEPT {
     NFS<LNF>::operator=(static_cast<NFS<LNF>&&>(src));
-    // No additional fields to assign
+    // Steal the compiled formatter
+    delete fImpl;
+    fImpl = src.fImpl;
+    src.fImpl = nullptr;
     return *this;
 }
 

diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h
index b75d8e6432f52d9cc15be66840f791c431e0cda3..61ef984077351fd8532354d52360d8742e99d992 100644 (file)
--- a/icu4c/source/i18n/unicode/numberformatter.h
+++ b/icu4c/source/i18n/unicode/numberformatter.h
@@ -2358,6 +2358,8 @@ class U_I18N_API LocalizedNumberFormatter
 
     LocalizedNumberFormatter(impl::MacroProps &&macros, const Locale &locale);
 
+    void clear();
+
     void lnfMoveHelper(LocalizedNumberFormatter&& src);
 
     /**

diff --git a/icu4c/source/i18n/unicode/numberrangeformatter.h b/icu4c/source/i18n/unicode/numberrangeformatter.h
index 5b9f264f9bf1f46203a76cfd1543258eb4c53c21..6e50daae15adfa40db30efc7708eb49ebb660c68 100644 (file)
--- a/icu4c/source/i18n/unicode/numberrangeformatter.h
+++ b/icu4c/source/i18n/unicode/numberrangeformatter.h
@@ -625,6 +625,8 @@ class U_I18N_API LocalizedNumberRangeFormatter
 
     LocalizedNumberRangeFormatter(impl::RangeMacroProps &&macros, const Locale &locale);
 
+    void clear();
+
     // To give the fluent setters access to this class's constructor:
     friend class NumberRangeFormatterSettings<UnlocalizedNumberRangeFormatter>;
     friend class NumberRangeFormatterSettings<LocalizedNumberRangeFormatter>;

diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h
index 0d0f7dec81575c52dfcb79e93406e48ac98e7127..49c2d4411a9bed357933da2be3379972f182a202 100644 (file)
--- a/icu4c/source/test/intltest/numbertest.h
+++ b/icu4c/source/test/intltest/numbertest.h
@@ -257,6 +257,7 @@ class NumberRangeFormatterTest : public IntlTest {
     void testIdentity();
     void testDifferentFormatters();
     void testPlurals();
+    void testCopyMove();
 
     void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = 0);
 

diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index 0604a9d9db12a47e01a1d267e273e84f645cfe4d..ec8da929244fe5380de9688e132f6b3d191e457a 100644 (file)
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -2372,8 +2372,15 @@ void NumberFormatterApiTest::copyMove() {
     assertTrue("[constructor] Source should be reset after move", l1.getCompiled() == nullptr);
 
     // Reset l1 and l2 to check for macro-props copying for behavior testing
+    // Make the test more interesting: also warm them up with a compiled formatter.
     l1 = NumberFormatter::withLocale("en");
+    l1.formatInt(1, status);
+    l1.formatInt(1, status);
+    l1.formatInt(1, status);
     l2 = NumberFormatter::withLocale("en");
+    l2.formatInt(1, status);
+    l2.formatInt(1, status);
+    l2.formatInt(1, status);
 
     // Copy assignment
     l1 = l3;

diff --git a/icu4c/source/test/intltest/numbertest_range.cpp b/icu4c/source/test/intltest/numbertest_range.cpp
index 1199f886545804f396310f63f19c66cb3fe3c90e..30f033d059c86758804d803a14b893c1daf3017c 100644 (file)
--- a/icu4c/source/test/intltest/numbertest_range.cpp
+++ b/icu4c/source/test/intltest/numbertest_range.cpp
@@ -48,6 +48,7 @@ void NumberRangeFormatterTest::runIndexedTest(int32_t index, UBool exec, const c
         TESTCASE_AUTO(testIdentity);
         TESTCASE_AUTO(testDifferentFormatters);
         TESTCASE_AUTO(testPlurals);
+        TESTCASE_AUTO(testCopyMove);
     TESTCASE_AUTO_END;
 }
 
@@ -706,6 +707,48 @@ void NumberRangeFormatterTest::testPlurals() {
     }
 }
 
+void NumberRangeFormatterTest::testCopyMove() {
+    IcuTestErrorCode status(*this, "testCopyMove");
+
+    // Default constructors
+    LocalizedNumberRangeFormatter l1;
+    assertEquals("Initial behavior", u"1–5", l1.formatFormattableRange(1, 5, status).toString(status));
+    if (status.errDataIfFailureAndReset()) { return; }
+
+    // Setup
+    l1 = NumberRangeFormatter::withLocale("fr-FR")
+        .numberFormatterBoth(NumberFormatter::with().unit(USD));
+    assertEquals("Currency behavior", u"1,00–5,00 $US", l1.formatFormattableRange(1, 5, status).toString(status));
+
+    // Copy constructor
+    LocalizedNumberRangeFormatter l2 = l1;
+    assertEquals("Copy constructor", u"1,00–5,00 $US", l2.formatFormattableRange(1, 5, status).toString(status));
+
+    // Move constructor
+    LocalizedNumberRangeFormatter l3 = std::move(l1);
+    assertEquals("Move constructor", u"1,00–5,00 $US", l3.formatFormattableRange(1, 5, status).toString(status));
+
+    // Reset objects for assignment tests
+    l1 = NumberRangeFormatter::withLocale("en-us");
+    l2 = NumberRangeFormatter::withLocale("en-us");
+    assertEquals("Rest behavior, l1", u"1–5", l1.formatFormattableRange(1, 5, status).toString(status));
+    assertEquals("Rest behavior, l2", u"1–5", l2.formatFormattableRange(1, 5, status).toString(status));
+
+    // Copy assignment
+    l1 = l3;
+    assertEquals("Copy constructor", u"1,00–5,00 $US", l1.formatFormattableRange(1, 5, status).toString(status));
+
+    // Move assignment
+    l2 = std::move(l3);
+    assertEquals("Copy constructor", u"1,00–5,00 $US", l2.formatFormattableRange(1, 5, status).toString(status));
+
+    // FormattedNumberRange
+    FormattedNumberRange result = l1.formatFormattableRange(1, 5, status);
+    assertEquals("FormattedNumberRange move constructor", u"1,00–5,00 $US", result.toString(status));
+    result = l1.formatFormattableRange(3, 6, status);
+    assertEquals("FormattedNumberRange move assignment", u"3,00–6,00 $US", result.toString(status));
+}
+
 void  NumberRangeFormatterTest::assertFormatRange(
       const char16_t* message,
       const UnlocalizedNumberRangeFormatter& f,