Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU-21383 Fix memory problem in FormattedStringBuilder #1450

Merged
merged 1 commit into from
Nov 11, 2020
Merged

ICU-21383 Fix memory problem in FormattedStringBuilder #1450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions icu4c/source/i18n/formattedval_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ class U_I18N_API FormattedValueStringBuilderImpl : public UMemory, public Format
* 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;
Expand Down
28 changes: 22 additions & 6 deletions icu4c/source/i18n/formattedval_sbimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
6 changes: 3 additions & 3 deletions icu4c/source/i18n/listformatter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ class FormattedListBuilder {
start,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->appendSpanInfo(0, start.length());
data->appendSpanInfo(0, start.length(), status);
}
}

Expand Down Expand Up @@ -603,7 +603,7 @@ class FormattedListBuilder {
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},
Expand All @@ -622,7 +622,7 @@ class FormattedListBuilder {
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]),
Expand Down
6 changes: 6 additions & 0 deletions icu4c/source/test/intltest/formattedvaluetest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand Down
54 changes: 54 additions & 0 deletions icu4c/source/test/intltest/listformattertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,12 @@ else if (cfpos.matchesField((Field) _field, null)) {
}

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;
}

Expand Down
4 changes: 3 additions & 1 deletion icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,9 @@ public void setInt64IterationContext(long context) {
*/
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;
Expand Down
6 changes: 6 additions & 0 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
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) {
Expand All @@ -238,6 +240,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
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
Expand Down Expand Up @@ -267,6 +271,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
}
}

Expand Down
82 changes: 80 additions & 2 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,8 @@ private boolean isDefaultLocaleEnglishLike() {

@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 = {
Expand All @@ -244,6 +243,85 @@ public void TestFormattedValue() {
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
Expand Down