From fa30c0eeb49ff6af1055699f7114ac6a483e5c86 Mon Sep 17 00:00:00 2001 From: Andy Heninger Date: Thu, 11 Nov 2021 16:12:44 -0800 Subject: [PATCH] ICU-21763 UVector cleanup, continued. Revise uses of UVector in the next batch of files 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/common/normalizer2impl.cpp | 11 ++- icu4c/source/common/serv.cpp | 5 +- icu4c/source/common/servls.cpp | 3 +- icu4c/source/common/servnotf.cpp | 8 +- icu4c/source/i18n/alphaindex.cpp | 88 ++++++++++------------ icu4c/source/i18n/collationdatabuilder.cpp | 7 +- icu4c/source/i18n/rbt_set.cpp | 47 ++++++------ icu4c/source/i18n/uspoof_conf.cpp | 43 ++++++----- icu4c/source/i18n/uspoof_conf.h | 11 +-- icu4c/source/test/intltest/icusvtst.cpp | 2 +- 10 files changed, 109 insertions(+), 116 deletions(-) diff --git a/icu4c/source/common/normalizer2impl.cpp b/icu4c/source/common/normalizer2impl.cpp index 5bfd49e8cb9..e6bd75e7173 100644 --- a/icu4c/source/common/normalizer2impl.cpp +++ b/icu4c/source/common/normalizer2impl.cpp @@ -2496,15 +2496,18 @@ void CanonIterData::addToStartSet(UChar32 origin, UChar32 decompLead, UErrorCode // origin is not the first character, or it is U+0000. UnicodeSet *set; if((canonValue&CANON_HAS_SET)==0) { - set=new UnicodeSet; - if(set==NULL) { - errorCode=U_MEMORY_ALLOCATION_ERROR; + LocalPointer lpSet(new UnicodeSet, errorCode); + set=lpSet.getAlias(); + if(U_FAILURE(errorCode)) { return; } UChar32 firstOrigin=(UChar32)(canonValue&CANON_VALUE_MASK); canonValue=(canonValue&~CANON_VALUE_MASK)|CANON_HAS_SET|(uint32_t)canonStartSets.size(); umutablecptrie_set(mutableTrie, decompLead, canonValue, &errorCode); - canonStartSets.addElementX(set, errorCode); + canonStartSets.adoptElement(lpSet.orphan(), errorCode); + if (U_FAILURE(errorCode)) { + return; + } if(firstOrigin!=0) { set->add(firstOrigin); } diff --git a/icu4c/source/common/serv.cpp b/icu4c/source/common/serv.cpp index 0c54a4dce99..c26dbca1a9c 100644 --- a/icu4c/source/common/serv.cpp +++ b/icu4c/source/common/serv.cpp @@ -625,10 +625,7 @@ ICUService::getVisibleIDs(UVector& result, const UnicodeString* matchID, UErrorC } } - LocalPointer idClone(new UnicodeString(*id), status); - if (U_SUCCESS(status) && idClone->isBogus()) { - status = U_MEMORY_ALLOCATION_ERROR; - } + LocalPointer idClone(id->clone(), status); result.adoptElement(idClone.orphan(), status); } delete fallbackKey; diff --git a/icu4c/source/common/servls.cpp b/icu4c/source/common/servls.cpp index 7108afd4a52..98f0a8a12b0 100644 --- a/icu4c/source/common/servls.cpp +++ b/icu4c/source/common/servls.cpp @@ -179,7 +179,8 @@ private: length = other._ids.size(); for(i = 0; i < length; ++i) { - _ids.addElementX(((UnicodeString *)other._ids.elementAt(i))->clone(), status); + LocalPointer clonedId(((UnicodeString *)other._ids.elementAt(i))->clone(), status); + _ids.adoptElement(clonedId.orphan(), status); } if(U_SUCCESS(status)) { diff --git a/icu4c/source/common/servnotf.cpp b/icu4c/source/common/servnotf.cpp index 342e0d9f24d..10183d3a2dc 100644 --- a/icu4c/source/common/servnotf.cpp +++ b/icu4c/source/common/servnotf.cpp @@ -49,7 +49,11 @@ ICUNotifier::addListener(const EventListener* l, UErrorCode& status) if (acceptsListener(*l)) { Mutex lmx(¬ifyLock); if (listeners == NULL) { - listeners = new UVector(5, status); + LocalPointer lpListeners(new UVector(5, status), status); + if (U_FAILURE(status)) { + return; + } + listeners = lpListeners.orphan(); } else { for (int i = 0, e = listeners->size(); i < e; ++i) { const EventListener* el = (const EventListener*)(listeners->elementAt(i)); @@ -59,7 +63,7 @@ ICUNotifier::addListener(const EventListener* l, UErrorCode& status) } } - listeners->addElementX((void*)l, status); // cast away const + listeners->addElement((void*)l, status); // cast away const } #ifdef NOTIFIER_DEBUG else { diff --git a/icu4c/source/i18n/alphaindex.cpp b/icu4c/source/i18n/alphaindex.cpp index 34407f677a6..88d63675a7c 100644 --- a/icu4c/source/i18n/alphaindex.cpp +++ b/icu4c/source/i18n/alphaindex.cpp @@ -451,12 +451,11 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { bucketList->setDeleter(uprv_deleteUObject); // underflow bucket - Bucket *bucket = new Bucket(getUnderflowLabel(), emptyString_, U_ALPHAINDEX_UNDERFLOW); - if (bucket == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; + LocalPointer bucket(new Bucket(getUnderflowLabel(), emptyString_, U_ALPHAINDEX_UNDERFLOW), errorCode); + if (U_FAILURE(errorCode)) { return NULL; } - bucketList->addElementX(bucket, errorCode); + bucketList->adoptElement(bucket.orphan(), errorCode); if (U_FAILURE(errorCode)) { return NULL; } UnicodeString temp; @@ -481,28 +480,24 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { if (skippedScript && bucketList->size() > 1) { // We are skipping one or more scripts, // and we are not just getting out of the underflow label. - bucket = new Bucket(getInflowLabel(), inflowBoundary, U_ALPHAINDEX_INFLOW); - if (bucket == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; - return NULL; - } - bucketList->addElementX(bucket, errorCode); + bucket.adoptInsteadAndCheckErrorCode( + new Bucket(getInflowLabel(), inflowBoundary, U_ALPHAINDEX_INFLOW), errorCode); + bucketList->adoptElement(bucket.orphan(), errorCode); + if (U_FAILURE(errorCode)) { return nullptr; } } } // Add a bucket with the current label. - bucket = new Bucket(fixLabel(current, temp), current, U_ALPHAINDEX_NORMAL); - if (bucket == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; - return NULL; - } - bucketList->addElementX(bucket, errorCode); + bucket.adoptInsteadAndCheckErrorCode( + new Bucket(fixLabel(current, temp), current, U_ALPHAINDEX_NORMAL), errorCode); + bucketList->adoptElement(bucket.orphan(), errorCode); + if (U_FAILURE(errorCode)) { return nullptr; } // Remember ASCII and Pinyin buckets for Pinyin redirects. UChar c; if (current.length() == 1 && 0x41 <= (c = current.charAt(0)) && c <= 0x5A) { // A-Z - asciiBuckets[c - 0x41] = bucket; + asciiBuckets[c - 0x41] = (Bucket *)bucketList->lastElement(); } else if (current.length() == BASE_LENGTH + 1 && current.startsWith(BASE, BASE_LENGTH) && 0x41 <= (c = current.charAt(BASE_LENGTH)) && c <= 0x5A) { - pinyinBuckets[c - 0x41] = bucket; + pinyinBuckets[c - 0x41] = (Bucket *)bucketList->lastElement(); hasPinyin = TRUE; } // Check for multiple primary weights. @@ -526,15 +521,16 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { // to the previous single-character bucket. // For example, after ... Q R S Sch we add Sch\uFFFF->S // and after ... Q R S Sch Sch\uFFFF St we add St\uFFFF->S. - bucket = new Bucket(emptyString_, + bucket.adoptInsteadAndCheckErrorCode(new Bucket(emptyString_, UnicodeString(current).append((UChar)0xFFFF), - U_ALPHAINDEX_NORMAL); - if (bucket == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; + U_ALPHAINDEX_NORMAL), + errorCode); + if (U_FAILURE(errorCode)) { return NULL; } bucket->displayBucket_ = singleBucket; - bucketList->addElementX(bucket, errorCode); + bucketList->adoptElement(bucket.orphan(), errorCode); + if (U_FAILURE(errorCode)) { return nullptr; } hasInvisibleBuckets = TRUE; break; } @@ -553,12 +549,10 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { return bl; } // overflow bucket - bucket = new Bucket(getOverflowLabel(), *scriptUpperBoundary, U_ALPHAINDEX_OVERFLOW); - if (bucket == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; - return NULL; - } - bucketList->addElementX(bucket, errorCode); // final + bucket.adoptInsteadAndCheckErrorCode( + new Bucket(getOverflowLabel(), *scriptUpperBoundary, U_ALPHAINDEX_OVERFLOW), errorCode); + bucketList->adoptElement(bucket.orphan(), errorCode); // final + if (U_FAILURE(errorCode)) { return nullptr; } if (hasPinyin) { // Redirect Pinyin buckets. @@ -589,7 +583,7 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { int32_t i = bucketList->size() - 1; Bucket *nextBucket = getBucket(*bucketList, i); while (--i > 0) { - bucket = getBucket(*bucketList, i); + Bucket *bucket = getBucket(*bucketList, i); if (bucket->displayBucket_ != NULL) { continue; // skip invisible buckets } @@ -609,9 +603,9 @@ BucketList *AlphabeticIndex::createBucketList(UErrorCode &errorCode) const { // Do not call publicBucketList->setDeleter(): // This vector shares its objects with the bucketList. for (int32_t j = 0; j < bucketList->size(); ++j) { - bucket = getBucket(*bucketList, j); + Bucket *bucket = getBucket(*bucketList, j); if (bucket->displayBucket_ == NULL) { - publicBucketList->addElementX(bucket, errorCode); + publicBucketList->addElement(bucket, errorCode); } } if (U_FAILURE(errorCode)) { return NULL; } @@ -679,13 +673,13 @@ void AlphabeticIndex::initBuckets(UErrorCode &errorCode) { bucket = bucket->displayBucket_; } if (bucket->records_ == NULL) { - bucket->records_ = new UVector(errorCode); - if (bucket->records_ == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; + LocalPointer records(new UVector(errorCode), errorCode); + if (U_FAILURE(errorCode)) { return; } + bucket->records_ = records.orphan(); } - bucket->records_->addElementX(r, errorCode); + bucket->records_->addElement(r, errorCode); } } @@ -1011,12 +1005,11 @@ UVector *AlphabeticIndex::firstStringsInScript(UErrorCode &status) { // and the one for unassigned implicit weights (Cn). continue; } - UnicodeString *s = new UnicodeString(boundary); - if (s == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - return NULL; + LocalPointer s(new UnicodeString(boundary), status); + dest->adoptElement(s.orphan(), status); + if (U_FAILURE(status)) { + return nullptr; } - dest->addElementX(s, status); } return dest.orphan(); } @@ -1067,19 +1060,18 @@ AlphabeticIndex & AlphabeticIndex::addRecord(const UnicodeString &name, const vo return *this; } if (inputList_ == NULL) { - inputList_ = new UVector(status); - if (inputList_ == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; + LocalPointer inputList(new UVector(status), status); + if (U_FAILURE(status)) { return *this; } + inputList_ = inputList.orphan(); inputList_->setDeleter(alphaIndex_deleteRecord); } - Record *r = new Record(name, data); - if (r == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; + LocalPointer r(new Record(name, data), status); + inputList_->adoptElement(r.orphan(), status); + if (U_FAILURE(status)) { return *this; } - inputList_->addElementX(r, status); clearBuckets(); //std::string ss; //std::string ss2; diff --git a/icu4c/source/i18n/collationdatabuilder.cpp b/icu4c/source/i18n/collationdatabuilder.cpp index 25050aa777e..b10de993c27 100644 --- a/icu4c/source/i18n/collationdatabuilder.cpp +++ b/icu4c/source/i18n/collationdatabuilder.cpp @@ -522,12 +522,11 @@ CollationDataBuilder::addConditionalCE32(const UnicodeString &context, uint32_t errorCode = U_BUFFER_OVERFLOW_ERROR; return -1; } - ConditionalCE32 *cond = new ConditionalCE32(context, ce32); - if(cond == NULL) { - errorCode = U_MEMORY_ALLOCATION_ERROR; + LocalPointer cond(new ConditionalCE32(context, ce32), errorCode); + conditionalCE32s.adoptElement(cond.orphan(), errorCode); + if(U_FAILURE(errorCode)) { return -1; } - conditionalCE32s.addElementX(cond, errorCode); return index; } diff --git a/icu4c/source/i18n/rbt_set.cpp b/icu4c/source/i18n/rbt_set.cpp index abc4413c2c6..6835c03a698 100644 --- a/icu4c/source/i18n/rbt_set.cpp +++ b/icu4c/source/i18n/rbt_set.cpp @@ -163,16 +163,13 @@ U_NAMESPACE_BEGIN /** * Construct a new empty rule set. */ -TransliterationRuleSet::TransliterationRuleSet(UErrorCode& status) : UMemory() { - ruleVector = new UVector(&_deleteRule, NULL, status); +TransliterationRuleSet::TransliterationRuleSet(UErrorCode& status) : + UMemory(), ruleVector(nullptr), rules(nullptr), index {}, maxContextLength(0) { + LocalPointer lpRuleVector(new UVector(_deleteRule, nullptr, status), status); if (U_FAILURE(status)) { return; } - if (ruleVector == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - } - rules = NULL; - maxContextLength = 0; + ruleVector = lpRuleVector.orphan(); } /** @@ -180,27 +177,24 @@ TransliterationRuleSet::TransliterationRuleSet(UErrorCode& status) : UMemory() { */ TransliterationRuleSet::TransliterationRuleSet(const TransliterationRuleSet& other) : UMemory(other), - ruleVector(0), - rules(0), + ruleVector(nullptr), + rules(nullptr), maxContextLength(other.maxContextLength) { int32_t i, len; uprv_memcpy(index, other.index, sizeof(index)); UErrorCode status = U_ZERO_ERROR; - ruleVector = new UVector(&_deleteRule, NULL, status); - if (other.ruleVector != 0 && ruleVector != 0 && U_SUCCESS(status)) { + LocalPointer lpRuleVector(new UVector(_deleteRule, nullptr, status), status); + if (U_FAILURE(status)) { + return; + } + ruleVector = lpRuleVector.orphan(); + if (other.ruleVector != nullptr && U_SUCCESS(status)) { len = other.ruleVector->size(); for (i=0; ielementAt(i)); - // Null pointer test - if (tempTranslitRule == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - break; - } - ruleVector->addElementX(tempTranslitRule, status); - if (U_FAILURE(status)) { - break; - } + LocalPointer tempTranslitRule( + new TransliterationRule(*(TransliterationRule*)other.ruleVector->elementAt(i)), status); + ruleVector->adoptElement(tempTranslitRule.orphan(), status); } } if (other.rules != 0 && U_SUCCESS(status)) { @@ -247,11 +241,11 @@ int32_t TransliterationRuleSet::getMaximumContextLength(void) const { */ void TransliterationRuleSet::addRule(TransliterationRule* adoptedRule, UErrorCode& status) { + LocalPointer lpAdoptedRule(adoptedRule); + ruleVector->adoptElement(lpAdoptedRule.orphan(), status); if (U_FAILURE(status)) { - delete adoptedRule; return; } - ruleVector->addElementX(adoptedRule, status); int32_t len; if ((len = adoptedRule->getContextLength()) > maxContextLength) { @@ -316,7 +310,7 @@ void TransliterationRuleSet::freeze(UParseError& parseError,UErrorCode& status) for (j=0; j= 0) { if (indexValue[j] == x) { - v.addElementX(ruleVector->elementAt(j), status); + v.addElement(ruleVector->elementAt(j), status); } } else { // If the indexValue is < 0, then the first key character is @@ -325,13 +319,16 @@ void TransliterationRuleSet::freeze(UParseError& parseError,UErrorCode& status) // rarely, so we seldom treat this code path. TransliterationRule* r = (TransliterationRule*) ruleVector->elementAt(j); if (r->matchesIndexValue((uint8_t)x)) { - v.addElementX(r, status); + v.addElement(r, status); } } } } uprv_free(indexValue); index[256] = v.size(); + if (U_FAILURE(status)) { + return; + } /* Freeze things into an array. */ diff --git a/icu4c/source/i18n/uspoof_conf.cpp b/icu4c/source/i18n/uspoof_conf.cpp index 04081cabfb0..172c0711afb 100644 --- a/icu4c/source/i18n/uspoof_conf.cpp +++ b/icu4c/source/i18n/uspoof_conf.cpp @@ -63,23 +63,24 @@ U_NAMESPACE_USE // at the same time // -SPUString::SPUString(UnicodeString *s) { - fStr = s; +SPUString::SPUString(LocalPointer s) { + fStr = std::move(s); fCharOrStrTableIndex = 0; } SPUString::~SPUString() { - delete fStr; } -SPUStringPool::SPUStringPool(UErrorCode &status) : fVec(NULL), fHash(NULL) { - fVec = new UVector(status); - if (fVec == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; +SPUStringPool::SPUStringPool(UErrorCode &status) : fVec(nullptr), fHash(nullptr) { + LocalPointer vec(new UVector(status), status); + if (U_FAILURE(status)) { return; } + vec->setDeleter( + [](void *obj) {delete (SPUString *)obj;}); + fVec = vec.orphan(); fHash = uhash_open(uhash_hashUnicodeString, // key hash function uhash_compareUnicodeString, // Key Comparator NULL, // Value Comparator @@ -88,11 +89,6 @@ SPUStringPool::SPUStringPool(UErrorCode &status) : fVec(NULL), fHash(NULL) { SPUStringPool::~SPUStringPool() { - int i; - for (i=fVec->size()-1; i>=0; i--) { - SPUString *s = static_cast(fVec->elementAt(i)); - delete s; - } delete fVec; uhash_close(fHash); } @@ -135,18 +131,21 @@ void SPUStringPool::sort(UErrorCode &status) { SPUString *SPUStringPool::addString(UnicodeString *src, UErrorCode &status) { + LocalPointer lpSrc(src); + if (U_FAILURE(status)) { + return nullptr; + } SPUString *hashedString = static_cast(uhash_get(fHash, src)); - if (hashedString != NULL) { - delete src; - } else { - hashedString = new SPUString(src); - if (hashedString == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - return NULL; - } - uhash_put(fHash, src, hashedString, &status); - fVec->addElementX(hashedString, status); + if (hashedString != nullptr) { + return hashedString; + } + LocalPointer spuStr(new SPUString(std::move(lpSrc)), status); + hashedString = spuStr.getAlias(); + fVec->adoptElement(spuStr.orphan(), status); + if (U_FAILURE(status)) { + return nullptr; } + uhash_put(fHash, src, hashedString, &status); return hashedString; } diff --git a/icu4c/source/i18n/uspoof_conf.h b/icu4c/source/i18n/uspoof_conf.h index 600d7ea42a4..1eeecdfd5e4 100644 --- a/icu4c/source/i18n/uspoof_conf.h +++ b/icu4c/source/i18n/uspoof_conf.h @@ -39,11 +39,12 @@ U_NAMESPACE_BEGIN // Instances of SPUString exist during the compilation process only. struct SPUString : public UMemory { - UnicodeString *fStr; // The actual string. - int32_t fCharOrStrTableIndex; // Index into the final runtime data for this - // string (or, for length 1, the single string char - // itself, there being no string table entry for it.) - SPUString(UnicodeString *s); + LocalPointer fStr; // The actual string. + int32_t fCharOrStrTableIndex; // Index into the final runtime data for this + // string (or, for length 1, the single string char + // itself, there being no string table entry for it.) + + SPUString(LocalPointer s); ~SPUString(); }; diff --git a/icu4c/source/test/intltest/icusvtst.cpp b/icu4c/source/test/intltest/icusvtst.cpp index aa2d9ca05ce..cb863c5b71b 100644 --- a/icu4c/source/test/intltest/icusvtst.cpp +++ b/icu4c/source/test/intltest/icusvtst.cpp @@ -558,7 +558,7 @@ class TestMultipleKeyStringFactory : public ICUServiceFactory { , _factoryID(factoryID + ": ") { for (int i = 0; i < count; ++i) { - _ids.addElementX(new UnicodeString(ids[i]), _status); + _ids.adoptElement(new UnicodeString(ids[i]), _status); } } -- 2.40.0