ICU-21383 Fix memory problem in FormattedStringBuilder

diff --git a/icu4c/source/i18n/formattedval_impl.h b/icu4c/source/i18n/formattedval_impl.h
index 4cfb0f078afee6da460915ac12727720f17af65b..c0dec83ba1eb369aa45b03e2c31c3ac7227edec2 100644 (file)
--- a/icu4c/source/i18n/formattedval_impl.h
+++ b/icu4c/source/i18n/formattedval_impl.h
@@ -174,8 +174,8 @@ public:
      * spanValue: the index of the list item, for example.
      * length: the length of the span, used to split adjacent fields.
      */
-    void appendSpanInfo(int32_t spanValue, int32_t length);
-    void prependSpanInfo(int32_t spanValue, int32_t length);
+    void appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);
+    void prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);
 
 private:
     FormattedStringBuilder fString;

diff --git a/icu4c/source/i18n/formattedval_sbimpl.cpp b/icu4c/source/i18n/formattedval_sbimpl.cpp
index aad4438360aef5bb4b88972513d9d8a1c3a11690..84c2d00666c2be7ead5dec85ddcfb966dd3e1616 100644 (file)
--- a/icu4c/source/i18n/formattedval_sbimpl.cpp
+++ b/icu4c/source/i18n/formattedval_sbimpl.cpp
@@ -219,19 +219,35 @@ bool FormattedValueStringBuilderImpl::nextPositionImpl(ConstrainedFieldPosition&
     }
 
     U_ASSERT(currField == kUndefinedField);
+    // Always set the position to the end so that we don't revisit previous sections
+    cfpos.setState(
+        cfpos.getCategory(),
+        cfpos.getField(),
+        fString.fLength,
+        fString.fLength);
     return false;
 }
 
-void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length) {
-    if (spanIndices.getCapacity() <= spanValue) {
-        spanIndices.resize(spanValue * 2);
+void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
+    if (U_FAILURE(status)) { return; }
+    U_ASSERT(spanIndices.getCapacity() >= spanValue);
+    if (spanIndices.getCapacity() == spanValue) {
+        if (!spanIndices.resize(spanValue * 2, spanValue)) {
+            status = U_MEMORY_ALLOCATION_ERROR;
+            return;
+        }
     }
     spanIndices[spanValue] = {spanValue, length};
 }
 
-void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length) {
-    if (spanIndices.getCapacity() <= spanValue) {
-        spanIndices.resize(spanValue * 2);
+void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
+    if (U_FAILURE(status)) { return; }
+    U_ASSERT(spanIndices.getCapacity() >= spanValue);
+    if (spanIndices.getCapacity() == spanValue) {
+        if (!spanIndices.resize(spanValue * 2, spanValue)) {
+            status = U_MEMORY_ALLOCATION_ERROR;
+            return;
+        }
     }
     for (int32_t i = spanValue - 1; i >= 0; i--) {
         spanIndices[i+1] = spanIndices[i];

diff --git a/icu4c/source/i18n/listformatter.cpp b/icu4c/source/i18n/listformatter.cpp
index 2ee646f0171ef821d667236a8c5cf19faabcadfa..be0d16bc7f52b3b94083e19b81d125e4490f8a88 100644 (file)
--- a/icu4c/source/i18n/listformatter.cpp
+++ b/icu4c/source/i18n/listformatter.cpp
@@ -567,7 +567,7 @@ public:
                 start,
                 {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
                 status);
-            data->appendSpanInfo(0, start.length());
+            data->appendSpanInfo(0, start.length(), status);
         }
     }
 
@@ -603,7 +603,7 @@ public:
                 next,
                 {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
                 status);
-            data->appendSpanInfo(position, next.length());
+            data->appendSpanInfo(position, next.length(), status);
             data->getStringRef().append(
                 temp.tempSubString(offsets[1]),
                 {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD},
@@ -622,7 +622,7 @@ public:
                 next,
                 {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
                 status);
-            data->prependSpanInfo(position, next.length());
+            data->prependSpanInfo(position, next.length(), status);
             data->getStringRef().insert(
                 0,
                 temp.tempSubStringBetween(0, offsets[1]),

diff --git a/icu4c/source/test/intltest/formattedvaluetest.cpp b/icu4c/source/test/intltest/formattedvaluetest.cpp
index 8de225dfd00db8e5acef2941560d3e0f78b7b478..0edf42086737fe07da0cf2373b45d362d4ff0264 100644 (file)
--- a/icu4c/source/test/intltest/formattedvaluetest.cpp
+++ b/icu4c/source/test/intltest/formattedvaluetest.cpp
@@ -237,6 +237,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
     }
     UBool afterLoopResult = fv.nextPosition(cfpos, status);
     assertFalse(baseMessage + u"A after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
+    afterLoopResult = fv.nextPosition(cfpos, status);
+    assertFalse(baseMessage + u"A after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);
 
     // Check nextPosition constrained over each category one at a time
     for (int32_t category=0; category<UFIELD_CATEGORY_COUNT+1; category++) {
@@ -266,6 +268,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
         }
         UBool afterLoopResult = fv.nextPosition(cfpos, status);
         assertFalse(baseMessage + u"B after loop @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
+        afterLoopResult = fv.nextPosition(cfpos, status);
+        assertFalse(baseMessage + u"B after loop again @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
     }
 
     // Check nextPosition constrained over each field one at a time
@@ -300,6 +304,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
         }
         UBool afterLoopResult = fv.nextPosition(cfpos, status);
         assertFalse(baseMessage + u"C after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
+        afterLoopResult = fv.nextPosition(cfpos, status);
+        assertFalse(baseMessage + u"C after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);
     }
 }
 

diff --git a/icu4c/source/test/intltest/listformattertest.cpp b/icu4c/source/test/intltest/listformattertest.cpp
index c96e2a09c1600aa2c41d1c5eeee7a6d6d56956d2..513d63edff947e442597a41483de03b451d1fca6 100644 (file)
--- a/icu4c/source/test/intltest/listformattertest.cpp
+++ b/icu4c/source/test/intltest/listformattertest.cpp
@@ -551,6 +551,60 @@ void ListFormatterTest::TestFormattedValue() {
             expectedFieldPositions,
             UPRV_LENGTHOF(expectedFieldPositions));
     }
+
+    {
+        LocalPointer<ListFormatter> fmt(ListFormatter::createInstance("en", ULISTFMT_TYPE_UNITS, ULISTFMT_WIDTH_SHORT, status));
+        if (status.errIfFailureAndReset()) { return; }
+        const char16_t* message = u"ICU-21383 Long list";
+        const char16_t* expectedString = u"a, b, c, d, e, f, g, h, i";
+        const UnicodeString inputs[] = {
+            u"a",
+            u"b",
+            u"c",
+            u"d",
+            u"e",
+            u"f",
+            u"g",
+            u"h",
+            u"i",
+        };
+        FormattedList result = fmt->formatStringsToValue(inputs, UPRV_LENGTHOF(inputs), status);
+        static const UFieldPositionWithCategory expectedFieldPositions[] = {
+            // field, begin index, end index
+            {UFIELD_CATEGORY_LIST_SPAN, 0, 0, 1},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 0, 1},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 1, 3},
+            {UFIELD_CATEGORY_LIST_SPAN, 1, 3, 4},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 3, 4},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 4, 6},
+            {UFIELD_CATEGORY_LIST_SPAN, 2, 6, 7},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 6, 7},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 7, 9},
+            {UFIELD_CATEGORY_LIST_SPAN, 3, 9, 10},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 9, 10},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 10, 12},
+            {UFIELD_CATEGORY_LIST_SPAN, 4, 12, 13},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 12, 13},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 13, 15},
+            {UFIELD_CATEGORY_LIST_SPAN, 5, 15, 16},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 15, 16},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 16, 18},
+            {UFIELD_CATEGORY_LIST_SPAN, 6, 18, 19},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 18, 19},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 19, 21},
+            {UFIELD_CATEGORY_LIST_SPAN, 7, 21, 22},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 21, 22},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 22, 24},
+            {UFIELD_CATEGORY_LIST_SPAN, 8, 24, 25},
+            {UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 24, 25},
+            };
+        checkMixedFormattedValue(
+            message,
+            result,
+            expectedString,
+            expectedFieldPositions,
+            UPRV_LENGTHOF(expectedFieldPositions));
+    }
 }
 
 void ListFormatterTest::DoTheRealListStyleTesting(Locale locale,

diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
index e6c54a57afb9ff333424959920b72be673d2e9be..0c82d263254f417245c1c08f427a1599a438a939 100644 (file)
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
@@ -226,6 +226,12 @@ public class FormattedValueStringBuilderImpl {
         }
 
         assert currField == null;
+        // Always set the position to the end so that we don't revisit previous sections
+        cfpos.setState(
+            cfpos.getField(),
+            cfpos.getFieldValue(),
+            self.length,
+            self.length);
         return false;
     }
 

diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java b/icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java
index fd69112468bafe2f0d372e67e7fee4ca012142db..fac9453d61707d6e1b2239ab4b7525e76eed9a52 100644 (file)
--- a/icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java
@@ -295,7 +295,9 @@ public class ConstrainedFieldPosition {
      */
     public void setState(Field field, Object value, int start, int limit) {
         // Check matchesField only as an assertion (debug build)
-        assert matchesField(field, value);
+        if (field != null) {
+            assert matchesField(field, value);
+        }
 
         fField = field;
         fValue = value;

diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
index 1c155cb98d081de56be79c36e1afe2133c6ea2f7..51a39f47ed2a3bb2fda95b9b608799c9741cc83c 100644 (file)
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
@@ -215,6 +215,8 @@ public class FormattedValueTest {
         }
         boolean afterLoopResult = fv.nextPosition(cfpos);
         assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+        afterLoopResult = fv.nextPosition(cfpos);
+        assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
 
         // Check nextPosition constrained over each class one at a time
         for (Class<?> classConstraint : uniqueFieldClasses) {
@@ -238,6 +240,8 @@ public class FormattedValueTest {
             }
             afterLoopResult = fv.nextPosition(cfpos);
             assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+            afterLoopResult = fv.nextPosition(cfpos);
+            assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
         }
 
         // Check nextPosition constrained over an unrelated class
@@ -267,6 +271,8 @@ public class FormattedValueTest {
             }
             afterLoopResult = fv.nextPosition(cfpos);
             assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+            afterLoopResult = fv.nextPosition(cfpos);
+            assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
         }
     }
 

diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
index b9c7b74b2d6d79169766306d7ad02cd883b1ec9b..92977524bc864d72546f6b41837a298ca73805b0 100644 (file)
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
@@ -217,9 +217,8 @@ public class ListFormatterTest extends TestFmwk {
 
     @Test
     public void TestFormattedValue() {
-        ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);
-
         {
+            ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);
             String message = "Field position test 1";
             String expectedString = "hello, wonderful, and world";
             String[] inputs = {
@@ -244,6 +243,85 @@ public class ListFormatterTest extends TestFmwk {
                 expectedString,
                 expectedFieldPositions);
         }
+
+        {
+            ListFormatter fmt = ListFormatter.getInstance(ULocale.CHINESE, Type.UNITS, Width.SHORT);
+            String message = "Field position test 2 (ICU-21340)";
+            String expectedString = "aabbbbbbbccc";
+            String inputs[] = {
+                "aa",
+                "bbbbbbb",
+                "ccc"
+            };
+            FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
+            Object[][] expectedFieldPositions = {
+                // field, begin index, end index
+                {ListFormatter.SpanField.LIST_SPAN, 0, 2, 0},
+                {ListFormatter.Field.ELEMENT, 0, 2},
+                {ListFormatter.SpanField.LIST_SPAN, 2, 9, 1},
+                {ListFormatter.Field.ELEMENT, 2, 9},
+                {ListFormatter.SpanField.LIST_SPAN, 9, 12, 2},
+                {ListFormatter.Field.ELEMENT, 9, 12}};
+            if (!logKnownIssue("21351", "Java still coalesces adjacent elements")) {
+                FormattedValueTest.checkFormattedValue(
+                    message,
+                    result,
+                    expectedString,
+                    expectedFieldPositions);
+            }
+        }
+    
+        {
+            ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH, Type.UNITS, Width.SHORT);
+            String message = "ICU-21383 Long list";
+            String expectedString = "a, b, c, d, e, f, g, h, i";
+            String inputs[] = {
+                "a",
+                "b",
+                "c",
+                "d",
+                "e",
+                "f",
+                "g",
+                "h",
+                "i",
+            };
+            FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
+            Object[][] expectedFieldPositions = {
+                // field, begin index, end index
+                {ListFormatter.SpanField.LIST_SPAN, 0, 1, 0},
+                {ListFormatter.Field.ELEMENT, 0, 1},
+                {ListFormatter.Field.LITERAL, 1, 3},
+                {ListFormatter.SpanField.LIST_SPAN, 3, 4, 1},
+                {ListFormatter.Field.ELEMENT, 3, 4},
+                {ListFormatter.Field.LITERAL, 4, 6},
+                {ListFormatter.SpanField.LIST_SPAN, 6, 7, 2},
+                {ListFormatter.Field.ELEMENT, 6, 7},
+                {ListFormatter.Field.LITERAL, 7, 9},
+                {ListFormatter.SpanField.LIST_SPAN, 9, 10, 3},
+                {ListFormatter.Field.ELEMENT, 9, 10},
+                {ListFormatter.Field.LITERAL, 10, 12},
+                {ListFormatter.SpanField.LIST_SPAN, 12, 13, 4},
+                {ListFormatter.Field.ELEMENT, 12, 13},
+                {ListFormatter.Field.LITERAL, 13, 15},
+                {ListFormatter.SpanField.LIST_SPAN, 15, 16, 5},
+                {ListFormatter.Field.ELEMENT, 15, 16},
+                {ListFormatter.Field.LITERAL, 16, 18},
+                {ListFormatter.SpanField.LIST_SPAN, 18, 19, 6},
+                {ListFormatter.Field.ELEMENT, 18, 19},
+                {ListFormatter.Field.LITERAL, 19, 21},
+                {ListFormatter.SpanField.LIST_SPAN, 21, 22, 7},
+                {ListFormatter.Field.ELEMENT, 21, 22},
+                {ListFormatter.Field.LITERAL, 22, 24},
+                {ListFormatter.SpanField.LIST_SPAN, 24, 25, 8},
+                {ListFormatter.Field.ELEMENT, 24, 25},
+                };
+            FormattedValueTest.checkFormattedValue(
+                message,
+                result,
+                expectedString,
+                expectedFieldPositions);
+        }
     }
 
     @Test