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-20036 CurrencyPluralInfo class improve handling of OOM errors #17

Merged
merged 8 commits into from
Aug 6, 2018
Merged

ICU-20036 CurrencyPluralInfo class improve handling of OOM errors #17

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8941fb7
ICU-20036 CurrencyPluralInfo class doesn't always check/handle OOM er…
jefgen Jul 20, 2018
8fe690c
Use LocalPointer instead of raw new/delete.
jefgen Jul 26, 2018
03445f0
Using nullptr instead of NULL.
jefgen Jul 26, 2018
4ae065f
Inspired by Andy's review feedback PluralRules changes, this change s…
jefgen Aug 1, 2018
cfea74f
Review feedback from Shane, should be more consistent about not check…
jefgen Aug 3, 2018
74a9ff3
Thanks to Shane for the review. Using LocalUResourceBundlePointer in …
jefgen Aug 3, 2018
473b4dc
Review feedback from Shane. No need for the explicit nullptr assignme…
jefgen Aug 3, 2018
a073cfe
Reduce memory usage by using the same UResourceBundle instead of allo…
jefgen Aug 4, 2018
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
150 changes: 106 additions & 44 deletions icu4c/source/i18n/currpinf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,25 @@ static const char gCurrUnitPtnTag[]="CurrencyUnitPatterns";
CurrencyPluralInfo::CurrencyPluralInfo(UErrorCode& status)
: fPluralCountToCurrencyUnitPattern(NULL),
fPluralRules(NULL),
fLocale(NULL) {
fLocale(NULL),
fInternalStatus(U_ZERO_ERROR) {
initialize(Locale::getDefault(), status);
}

CurrencyPluralInfo::CurrencyPluralInfo(const Locale& locale, UErrorCode& status)
: fPluralCountToCurrencyUnitPattern(NULL),
fPluralRules(NULL),
fLocale(NULL) {
fLocale(NULL),
fInternalStatus(U_ZERO_ERROR) {
initialize(locale, status);
}

CurrencyPluralInfo::CurrencyPluralInfo(const CurrencyPluralInfo& info)
: UObject(info),
fPluralCountToCurrencyUnitPattern(NULL),
fPluralRules(NULL),
fLocale(NULL) {
fLocale(NULL),
fInternalStatus(U_ZERO_ERROR) {
*this = info;
}

Expand All @@ -93,24 +96,44 @@ CurrencyPluralInfo::operator=(const CurrencyPluralInfo& info) {
return *this;
}

fInternalStatus = info.fInternalStatus;
if (U_FAILURE(fInternalStatus)) {
// bail out early if the object we were copying from was already 'invalid'.
return *this;
}

deleteHash(fPluralCountToCurrencyUnitPattern);
UErrorCode status = U_ZERO_ERROR;
fPluralCountToCurrencyUnitPattern = initHash(status);

fPluralCountToCurrencyUnitPattern = initHash(fInternalStatus);
copyHash(info.fPluralCountToCurrencyUnitPattern,
fPluralCountToCurrencyUnitPattern, status);
if ( U_FAILURE(status) ) {
fPluralCountToCurrencyUnitPattern, fInternalStatus);
if ( U_FAILURE(fInternalStatus) ) {
return *this;
}

delete fPluralRules;
delete fLocale;
if (info.fPluralRules) {
fPluralRules = info.fPluralRules->clone();
if (fPluralRules == nullptr) {
fInternalStatus = U_MEMORY_ALLOCATION_ERROR;
return *this;
}
} else {
fPluralRules = NULL;
}
if (info.fLocale) {
fLocale = info.fLocale->clone();
if (fLocale == nullptr) {
fInternalStatus = U_MEMORY_ALLOCATION_ERROR;
return *this;
}
// If the other locale wasn't bogus, but our clone'd locale is bogus, then OOM happened
// during the call to clone().
if (!info.fLocale->isBogus() && fLocale->isBogus()) {
fInternalStatus = U_MEMORY_ALLOCATION_ERROR;
return *this;
}
} else {
fLocale = NULL;
}
Expand Down Expand Up @@ -148,7 +171,14 @@ CurrencyPluralInfo::operator==(const CurrencyPluralInfo& info) const {

CurrencyPluralInfo*
CurrencyPluralInfo::clone() const {
return new CurrencyPluralInfo(*this);
CurrencyPluralInfo* newObj = new CurrencyPluralInfo(*this);
// Since clone doesn't have a 'status' parameter, the best we can do is return nullptr
// if the new object was not full constructed properly (an error occurred).
if (newObj != nullptr && U_FAILURE(newObj->fInternalStatus)) {
delete newObj;
newObj = nullptr;
}
return newObj;
}

const PluralRules*
Expand Down Expand Up @@ -222,10 +252,24 @@ CurrencyPluralInfo::initialize(const Locale& loc, UErrorCode& status) {
if (U_FAILURE(status)) {
return;
}
delete fLocale;
fLocale = loc.clone();
if (fLocale) {
Copy link
Member

@sffc sffc Aug 2, 2018 •
edited by jefgen
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some methods, like operator=, you delete without checking first, but here you first check if the pointer is not nullptr before calling delete. Personally I like the first style better, but at a minimum you should probably be consistent in your style. #Resolved

Copy link
Member Author

@jefgen jefgen Aug 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Shane. I was trying to mimic the style within each function (if it checked for nullptr, then I would too. If it didn't then I didn't).
However, since I'm already making a bunch of changes it would probably be better to be consistent.
Since calling delete on nullptr is safe, it seems like it would be cleaner to omit the checks.

In reply to: 207376490 [](ancestors = 207376490)

delete fLocale;
fLocale = nullptr;
}
if (fPluralRules) {
delete fPluralRules;
fPluralRules = nullptr;
}
fLocale = loc.clone();
if (fLocale == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
// If the locale passed in wasn't bogus, but our clone is bogus, then OOM happened
// during the call to loc.clone().
if (!loc.isBogus() && fLocale->isBogus()) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
fPluralRules = PluralRules::forLocale(loc, status);
setupCurrencyPluralPattern(loc, status);
Expand All @@ -246,7 +290,10 @@ CurrencyPluralInfo::setupCurrencyPluralPattern(const Locale& loc, UErrorCode& st
return;
}

NumberingSystem *ns = NumberingSystem::createInstance(loc,status);
NumberingSystem *ns = NumberingSystem::createInstance(loc, status);
if (U_FAILURE(status)) {
return;
}
UErrorCode ec = U_ZERO_ERROR;
UResourceBundle *rb = ures_open(NULL, loc.getName(), &ec);
UResourceBundle *numElements = ures_getByKeyWithFallback(rb, gNumberElementsTag, NULL, &ec);
Expand Down Expand Up @@ -284,6 +331,10 @@ CurrencyPluralInfo::setupCurrencyPluralPattern(const Locale& loc, UErrorCode& st
delete ns;

if (U_FAILURE(ec)) {
// If OOM occurred during the above code, then we want to report that back to the caller.
if (ec == U_MEMORY_ALLOCATION_ERROR) {
status = ec;
}
return;
}

Expand All @@ -296,41 +347,51 @@ CurrencyPluralInfo::setupCurrencyPluralPattern(const Locale& loc, UErrorCode& st
StringEnumeration* keywords = fPluralRules->getKeywords(ec);
if (U_SUCCESS(ec)) {
const char* pluralCount;
while ((pluralCount = keywords->next(NULL, ec)) != NULL) {
if ( U_SUCCESS(ec) ) {
int32_t ptnLen;
UErrorCode err = U_ZERO_ERROR;
const UChar* patternChars = ures_getStringByKeyWithFallback(
currencyRes, pluralCount, &ptnLen, &err);
if (U_SUCCESS(err) && ptnLen > 0) {
UnicodeString* pattern = new UnicodeString(patternChars, ptnLen);
while (((pluralCount = keywords->next(NULL, ec)) != NULL) && U_SUCCESS(ec)) {
int32_t ptnLen;
UErrorCode err = U_ZERO_ERROR;
const UChar* patternChars = ures_getStringByKeyWithFallback(currencyRes, pluralCount, &ptnLen, &err);
if (err == U_MEMORY_ALLOCATION_ERROR || patternChars == nullptr) {
ec = err;
break;
}
if (U_SUCCESS(err) && ptnLen > 0) {
UnicodeString* pattern = new UnicodeString(patternChars, ptnLen);
if (pattern == nullptr) {
ec = U_MEMORY_ALLOCATION_ERROR;
break;
}
#ifdef CURRENCY_PLURAL_INFO_DEBUG
char result_1[1000];
pattern->extract(0, pattern->length(), result_1, "UTF-8");
std::cout << "pluralCount: " << pluralCount << "; pattern: " << result_1 << "\n";
char result_1[1000];
pattern->extract(0, pattern->length(), result_1, "UTF-8");
std::cout << "pluralCount: " << pluralCount << "; pattern: " << result_1 << "\n";
#endif
pattern->findAndReplace(UnicodeString(TRUE, gPart0, 3),
UnicodeString(numberStylePattern, numberStylePatternLen));
pattern->findAndReplace(UnicodeString(TRUE, gPart1, 3), UnicodeString(TRUE, gTripleCurrencySign, 3));

if (hasSeparator) {
UnicodeString negPattern(patternChars, ptnLen);
negPattern.findAndReplace(UnicodeString(TRUE, gPart0, 3),
UnicodeString(negNumberStylePattern, negNumberStylePatternLen));
negPattern.findAndReplace(UnicodeString(TRUE, gPart1, 3), UnicodeString(TRUE, gTripleCurrencySign, 3));
pattern->append(gNumberPatternSeparator);
pattern->append(negPattern);
}
pattern->findAndReplace(UnicodeString(TRUE, gPart0, 3),
UnicodeString(numberStylePattern, numberStylePatternLen));
pattern->findAndReplace(UnicodeString(TRUE, gPart1, 3), UnicodeString(TRUE, gTripleCurrencySign, 3));

if (hasSeparator) {
UnicodeString negPattern(patternChars, ptnLen);
negPattern.findAndReplace(UnicodeString(TRUE, gPart0, 3),
UnicodeString(negNumberStylePattern, negNumberStylePatternLen));
negPattern.findAndReplace(UnicodeString(TRUE, gPart1, 3), UnicodeString(TRUE, gTripleCurrencySign, 3));
pattern->append(gNumberPatternSeparator);
pattern->append(negPattern);
}
#ifdef CURRENCY_PLURAL_INFO_DEBUG
pattern->extract(0, pattern->length(), result_1, "UTF-8");
std::cout << "pluralCount: " << pluralCount << "; pattern: " << result_1 << "\n";
pattern->extract(0, pattern->length(), result_1, "UTF-8");
std::cout << "pluralCount: " << pluralCount << "; pattern: " << result_1 << "\n";
#endif

fPluralCountToCurrencyUnitPattern->put(UnicodeString(pluralCount, -1, US_INV), pattern, status);
}
// the 'pattern' object allocated above will be owned by the fPluralCountToCurrencyUnitPattern after the call to
// put(), even if the method returns failure;
fPluralCountToCurrencyUnitPattern->put(UnicodeString(pluralCount, -1, US_INV), pattern, status);
}
}
}
// If OOM occurred during the above code, then we want to report that back to the caller.
if (ec == U_MEMORY_ALLOCATION_ERROR) {
status = ec;
}
delete keywords;
ures_close(currencyRes);
ures_close(currRb);
Expand All @@ -339,8 +400,7 @@ CurrencyPluralInfo::setupCurrencyPluralPattern(const Locale& loc, UErrorCode& st


void
CurrencyPluralInfo::deleteHash(Hashtable* hTable)
{
CurrencyPluralInfo::deleteHash(Hashtable* hTable) {
if ( hTable == NULL ) {
return;
}
Expand All @@ -355,7 +415,6 @@ CurrencyPluralInfo::deleteHash(Hashtable* hTable)
hTable = NULL;
}


Hashtable*
CurrencyPluralInfo::initHash(UErrorCode& status) {
if ( U_FAILURE(status) ) {
Expand All @@ -374,7 +433,6 @@ CurrencyPluralInfo::initHash(UErrorCode& status) {
return hTable;
}


void
CurrencyPluralInfo::copyHash(const Hashtable* source,
Hashtable* target,
Expand All @@ -391,6 +449,11 @@ CurrencyPluralInfo::copyHash(const Hashtable* source,
const UHashTok valueTok = element->value;
const UnicodeString* value = (UnicodeString*)valueTok.pointer;
UnicodeString* copy = new UnicodeString(*value);
if (copy == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
// The HashTable owns the 'copy' object after the call to put().
target->put(UnicodeString(*key), copy, status);
if ( U_FAILURE(status) ) {
return;
Expand All @@ -399,7 +462,6 @@ CurrencyPluralInfo::copyHash(const Hashtable* source,
}
}


U_NAMESPACE_END

#endif
15 changes: 12 additions & 3 deletions icu4c/source/i18n/unicode/currpinf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// License & terms of use: http://www.unicode.org/copyright.html
/*
*******************************************************************************
* Copyright (C) 2009-2015, International Business Machines Corporation and *
* Copyright (C) 2009-2015, International Business Machines Corporation and *
* others. All Rights Reserved. *
*******************************************************************************
*/
Expand Down Expand Up @@ -240,18 +240,27 @@ class U_I18N_API CurrencyPluralInfo : public UObject {
/*
* The plural rule is used to format currency plural name,
* for example: "3.00 US Dollars".
* If there are 3 currency signs in the currency patttern,
* If there are 3 currency signs in the currency pattern,
* the 3 currency signs will be replaced by currency plural name.
*/
PluralRules* fPluralRules;

// locale
Locale* fLocale;

private:
/**
* An internal status variable used to indicate that the object is in an 'invalid' state.
* Used by copy constructor, the assignment operator and the clone method.
*/
UErrorCode fInternalStatus;
};


inline UBool
CurrencyPluralInfo::operator!=(const CurrencyPluralInfo& info) const { return !operator==(info); }
CurrencyPluralInfo::operator!=(const CurrencyPluralInfo& info) const {
return !operator==(info);
}

U_NAMESPACE_END

Expand Down