From 904cf62457de2440e8526deb75c95a3f7296f517 Mon Sep 17 00:00:00 2001 From: Andy Heninger Date: Fri, 19 Nov 2021 16:33:58 -0800 Subject: [PATCH] ICU-21763 UVector cleanup in Formatting Code Revise uses of UVector in Formatting related code to better handle memory allocation failures. This is one of an ongoing series of commits to address similar problems with UVector usage throughout ICU. The changes primarily involve switching uses of UVector::addElementX() to the new adoptElement() or addElement() functions, as appropriate, and using LocalPointers for tracking memory ownership. --- icu4c/source/i18n/dtfmtsym.cpp | 30 ++++++++++------------------ icu4c/source/i18n/dtptngen.cpp | 12 ++++++----- icu4c/source/i18n/msgfmt.cpp | 21 +++++++++---------- icu4c/source/i18n/msgfmt_impl.h | 4 ++-- icu4c/source/i18n/number_compact.cpp | 4 ++-- icu4c/source/i18n/numsys.cpp | 7 +------ icu4c/source/i18n/plurrule.cpp | 18 ++++------------- icu4c/source/i18n/tmutfmt.cpp | 4 ++-- 8 files changed, 39 insertions(+), 61 deletions(-) diff --git a/icu4c/source/i18n/dtfmtsym.cpp b/icu4c/source/i18n/dtfmtsym.cpp index ab5f2b612c1..134b919f06e 100644 --- a/icu4c/source/i18n/dtfmtsym.cpp +++ b/icu4c/source/i18n/dtfmtsym.cpp @@ -1574,26 +1574,20 @@ struct CalendarDataSink : public ResourceSink { errorCode); if (U_FAILURE(errorCode)) { return; } } - LocalPointer aliasRelativePathCopy(new UnicodeString(aliasRelativePath), errorCode); - resourcesToVisitNext->addElementX(aliasRelativePathCopy.getAlias(), errorCode); + LocalPointer aliasRelativePathCopy(aliasRelativePath.clone(), errorCode); + resourcesToVisitNext->adoptElement(aliasRelativePathCopy.orphan(), errorCode); if (U_FAILURE(errorCode)) { return; } - // Only release ownership after resourcesToVisitNext takes it (no error happened): - aliasRelativePathCopy.orphan(); continue; } else if (aliasType == SAME_CALENDAR) { // Register same-calendar alias if (arrays.get(aliasRelativePath) == NULL && maps.get(aliasRelativePath) == NULL) { - LocalPointer aliasRelativePathCopy(new UnicodeString(aliasRelativePath), errorCode); - aliasPathPairs.addElementX(aliasRelativePathCopy.getAlias(), errorCode); + LocalPointer aliasRelativePathCopy(aliasRelativePath.clone(), errorCode); + aliasPathPairs.adoptElement(aliasRelativePathCopy.orphan(), errorCode); if (U_FAILURE(errorCode)) { return; } - // Only release ownership after aliasPathPairs takes it (no error happened): - aliasRelativePathCopy.orphan(); - LocalPointer keyUStringCopy(new UnicodeString(keyUString), errorCode); - aliasPathPairs.addElementX(keyUStringCopy.getAlias(), errorCode); + LocalPointer keyUStringCopy(keyUString.clone(), errorCode); + aliasPathPairs.adoptElement(keyUStringCopy.orphan(), errorCode); if (U_FAILURE(errorCode)) { return; } - // Only release ownership after aliasPathPairs takes it (no error happened): - keyUStringCopy.orphan(); } continue; } @@ -1760,16 +1754,12 @@ struct CalendarDataSink : public ResourceSink { if (U_FAILURE(errorCode)) { return; } if (aliasType == SAME_CALENDAR) { // Store the alias path and the current path on aliasPathPairs - LocalPointer aliasRelativePathCopy(new UnicodeString(aliasRelativePath), errorCode); - aliasPathPairs.addElementX(aliasRelativePathCopy.getAlias(), errorCode); + LocalPointer aliasRelativePathCopy(aliasRelativePath.clone(), errorCode); + aliasPathPairs.adoptElement(aliasRelativePathCopy.orphan(), errorCode); if (U_FAILURE(errorCode)) { return; } - // Only release ownership after aliasPathPairs takes it (no error happened): - aliasRelativePathCopy.orphan(); - LocalPointer pathCopy(new UnicodeString(path), errorCode); - aliasPathPairs.addElementX(pathCopy.getAlias(), errorCode); + LocalPointer pathCopy(path.clone(), errorCode); + aliasPathPairs.adoptElement(pathCopy.orphan(), errorCode); if (U_FAILURE(errorCode)) { return; } - // Only release ownership after aliasPathPairs takes it (no error happened): - pathCopy.orphan(); // Drop the latest key on the path and continue path.retainBetween(0, pathLength); diff --git a/icu4c/source/i18n/dtptngen.cpp b/icu4c/source/i18n/dtptngen.cpp index 6aee1750f90..987d83663ac 100644 --- a/icu4c/source/i18n/dtptngen.cpp +++ b/icu4c/source/i18n/dtptngen.cpp @@ -2788,16 +2788,17 @@ DTSkeletonEnumeration::DTSkeletonEnumeration(PatternMap& patternMap, dtStrEnum t break; } if ( !isCanonicalItem(s) ) { - LocalPointer newElem(new UnicodeString(s), status); + LocalPointer newElem(s.clone(), status); if (U_FAILURE(status)) { return; } - fSkeletons->addElementX(newElem.getAlias(), status); + fSkeletons->addElement(newElem.getAlias(), status); if (U_FAILURE(status)) { fSkeletons.adoptInstead(nullptr); return; } - newElem.orphan(); // fSkeletons vector now owns the UnicodeString. + newElem.orphan(); // fSkeletons vector now owns the UnicodeString (although it + // does not use a deleter function to manage the ownership). } curElem = curElem->next.getAlias(); } @@ -2865,12 +2866,13 @@ DTRedundantEnumeration::add(const UnicodeString& pattern, UErrorCode& status) { if (U_FAILURE(status)) { return; } - fPatterns->addElementX(newElem.getAlias(), status); + fPatterns->addElement(newElem.getAlias(), status); if (U_FAILURE(status)) { fPatterns.adoptInstead(nullptr); return; } - newElem.orphan(); // fPatterns now owns the string. + newElem.orphan(); // fPatterns now owns the string, although a UVector + // deleter function is not used to manage that ownership. } const UnicodeString* diff --git a/icu4c/source/i18n/msgfmt.cpp b/icu4c/source/i18n/msgfmt.cpp index b8cb2e2ca56..13a5a089516 100644 --- a/icu4c/source/i18n/msgfmt.cpp +++ b/icu4c/source/i18n/msgfmt.cpp @@ -854,19 +854,21 @@ StringEnumeration* MessageFormat::getFormatNames(UErrorCode& status) { if (U_FAILURE(status)) return NULL; - UVector *fFormatNames = new UVector(status); + LocalPointer formatNames(new UVector(status), status); if (U_FAILURE(status)) { - status = U_MEMORY_ALLOCATION_ERROR; - return NULL; + return nullptr; } - fFormatNames->setDeleter(uprv_deleteUObject); + formatNames->setDeleter(uprv_deleteUObject); for (int32_t partIndex = 0; (partIndex = nextTopLevelArgStart(partIndex)) >= 0;) { - fFormatNames->addElementX(new UnicodeString(getArgName(partIndex + 1)), status); + LocalPointer name(getArgName(partIndex + 1).clone(), status); + formatNames->adoptElement(name.orphan(), status); + if (U_FAILURE(status)) return nullptr; } - StringEnumeration* nameEnumerator = new FormatNameEnumeration(fFormatNames, status); - return nameEnumerator; + LocalPointer nameEnumerator( + new FormatNameEnumeration(std::move(formatNames), status), status); + return U_SUCCESS(status) ? nameEnumerator.orphan() : nullptr; } // ------------------------------------- @@ -1912,9 +1914,9 @@ void MessageFormat::DummyFormat::parseObject(const UnicodeString&, } -FormatNameEnumeration::FormatNameEnumeration(UVector *fNameList, UErrorCode& /*status*/) { +FormatNameEnumeration::FormatNameEnumeration(LocalPointer nameList, UErrorCode& /*status*/) { pos=0; - fFormatNames = fNameList; + fFormatNames = std::move(nameList); } const UnicodeString* @@ -1936,7 +1938,6 @@ FormatNameEnumeration::count(UErrorCode& /*status*/) const { } FormatNameEnumeration::~FormatNameEnumeration() { - delete fFormatNames; } MessageFormat::PluralSelectorProvider::PluralSelectorProvider(const MessageFormat &mf, UPluralType t) diff --git a/icu4c/source/i18n/msgfmt_impl.h b/icu4c/source/i18n/msgfmt_impl.h index 57988389132..80cb8a691eb 100644 --- a/icu4c/source/i18n/msgfmt_impl.h +++ b/icu4c/source/i18n/msgfmt_impl.h @@ -26,7 +26,7 @@ U_NAMESPACE_BEGIN class FormatNameEnumeration : public StringEnumeration { public: - FormatNameEnumeration(UVector *fFormatNames, UErrorCode& status); + FormatNameEnumeration(LocalPointer fFormatNames, UErrorCode& status); virtual ~FormatNameEnumeration(); static UClassID U_EXPORT2 getStaticClassID(void); virtual UClassID getDynamicClassID(void) const override; @@ -35,7 +35,7 @@ public: virtual int32_t count(UErrorCode& status) const override; private: int32_t pos; - UVector *fFormatNames; + LocalPointer fFormatNames; }; U_NAMESPACE_END diff --git a/icu4c/source/i18n/number_compact.cpp b/icu4c/source/i18n/number_compact.cpp index 62692f444df..60cd7bedf66 100644 --- a/icu4c/source/i18n/number_compact.cpp +++ b/icu4c/source/i18n/number_compact.cpp @@ -157,8 +157,8 @@ void CompactData::getUniquePatterns(UVector &output, UErrorCode &status) const { } // The string was not found; add it to the UVector. - // ANDY: This requires a const_cast. Why? - output.addElementX(const_cast(pattern), status); + // Note: must cast off const from pattern to store it in a UVector, which expects (void *) + output.addElement(const_cast(pattern), status); continue_outer: continue; diff --git a/icu4c/source/i18n/numsys.cpp b/icu4c/source/i18n/numsys.cpp index 44aaf8e2a5f..934149039c5 100644 --- a/icu4c/source/i18n/numsys.cpp +++ b/icu4c/source/i18n/numsys.cpp @@ -313,12 +313,7 @@ U_CFUNC void initNumsysNames(UErrorCode &status) { } const char *nsName = ures_getKey(nsCurrent.getAlias()); LocalPointer newElem(new UnicodeString(nsName, -1, US_INV), status); - if (U_SUCCESS(status)) { - numsysNames->addElementX(newElem.getAlias(), status); - if (U_SUCCESS(status)) { - newElem.orphan(); // on success, the numsysNames vector owns newElem. - } - } + numsysNames->adoptElement(newElem.orphan(), status); } ures_close(numberingSystemsInfo); diff --git a/icu4c/source/i18n/plurrule.cpp b/icu4c/source/i18n/plurrule.cpp index d1918c46981..cb817042da8 100644 --- a/icu4c/source/i18n/plurrule.cpp +++ b/icu4c/source/i18n/plurrule.cpp @@ -1548,14 +1548,9 @@ PluralKeywordEnumeration::PluralKeywordEnumeration(RuleChain *header, UErrorCode UBool addKeywordOther = TRUE; RuleChain *node = header; while (node != nullptr) { - auto newElem = new UnicodeString(node->fKeyword); - if (newElem == nullptr) { - status = U_MEMORY_ALLOCATION_ERROR; - return; - } - fKeywordNames.addElementX(newElem, status); + LocalPointer newElem(node->fKeyword.clone(), status); + fKeywordNames.adoptElement(newElem.orphan(), status); if (U_FAILURE(status)) { - delete newElem; return; } if (0 == node->fKeyword.compare(PLURAL_KEYWORD_OTHER, 5)) { @@ -1565,14 +1560,9 @@ PluralKeywordEnumeration::PluralKeywordEnumeration(RuleChain *header, UErrorCode } if (addKeywordOther) { - auto newElem = new UnicodeString(PLURAL_KEYWORD_OTHER); - if (newElem == nullptr) { - status = U_MEMORY_ALLOCATION_ERROR; - return; - } - fKeywordNames.addElementX(newElem, status); + LocalPointer newElem(new UnicodeString(PLURAL_KEYWORD_OTHER), status); + fKeywordNames.adoptElement(newElem.orphan(), status); if (U_FAILURE(status)) { - delete newElem; return; } } diff --git a/icu4c/source/i18n/tmutfmt.cpp b/icu4c/source/i18n/tmutfmt.cpp index 057bb634ebb..f0335a81f50 100644 --- a/icu4c/source/i18n/tmutfmt.cpp +++ b/icu4c/source/i18n/tmutfmt.cpp @@ -320,14 +320,14 @@ void TimeUnitFormat::setup(UErrorCode& err) { initDataMembers(err); - UVector pluralCounts(0, uhash_compareUnicodeString, 6, err); + UVector pluralCounts(nullptr, uhash_compareUnicodeString, 6, err); LocalPointer keywords(getPluralRules().getKeywords(err), err); if (U_FAILURE(err)) { return; } UnicodeString* pluralCount; while ((pluralCount = const_cast(keywords->snext(err))) != NULL) { - pluralCounts.addElementX(pluralCount, err); + pluralCounts.addElement(pluralCount, err); } readFromCurrentLocale(UTMUTFMT_FULL_STYLE, gUnitsTag, pluralCounts, err); checkConsistency(UTMUTFMT_FULL_STYLE, gUnitsTag, err);