Icu 8144 #1370
Icu 8144 #1370
Conversation
sffc
added
the
after-release
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
Thank you, and sorry for the very much delayed review!
We need to think more about the lifecycle of the CurrencyDisplayNames objects. Who owns them? If you request one from the same locale multiple times in a row, are they saved in a cache? Although I don't generally like caches, I think this is a case where we might want to consider pulling in the UnifiedCache.
| if (pluralCategory == PluralMapBase::NONE) { | ||
| errorCode = U_UNSUPPORTED_ERROR; | ||
| } | ||
| if (uhash_get(pluralsData->pluralsStringTable, (int32_t *)pluralCategory) == NULL) { |
There was a problem hiding this comment.
I think you can unconditionally set the hashtable key here. The keys in pluralsTable will be unique.
| UBool isMissing = false; | ||
| UHashtable *pluralsStringTable = nullptr; | ||
|
|
||
| PluralsData(const UChar *isoCode) : isoCode(isoCode) {} |
There was a problem hiding this comment.
Please either:
- Change the type of the UHashtable field to LocalUHashtablePointer
- Keep the type as UHashtable but add a destructor to close the UHashtable
| CurrencyDisplayNames *instance = currencyDisplayDataCache; | ||
| if (instance == nullptr || strcmp((instance->locale)->getName(), loc->getName()) != 0 || | ||
| instance->noSubstitute != noSubstitute) { | ||
| Locale *internalLocale; |
| return getInstance(loc, false, errorCode); | ||
| } | ||
|
|
||
| const CurrencyDisplayNames *CurrencyDisplayNames::getInstance(const Locale *loc, UBool noSubstitute, |
There was a problem hiding this comment.
Here and elsewhere: I would take a Locale by const reference, not const pointer
| internalLocale = new Locale(*loc); | ||
| } | ||
| UErrorCode ec2 = U_ZERO_ERROR; | ||
| UResourceBundle *currencyResources = ures_open(U_ICUDATA_CURR, internalLocale->getName(), &ec2); |
| return nullptr; | ||
| } | ||
| instance = new CurrencyDisplayNames(internalLocale, currencyResources, noSubstitute); | ||
| currencyDisplayDataCache = instance; |
There was a problem hiding this comment.
This line isn't thread-safe. You also need to delete the old instance if present.
|
|
||
| const Locale *CurrencyDisplayNames::getLocale() { return locale; } | ||
|
|
||
| const UChar *CurrencyDisplayNames::getName(const UChar *isoCode, UErrorCode &errorCode) const { |
There was a problem hiding this comment.
Here and elsewhere: Why not return the data as a UnicodeString? It makes it easier to reason about who owns the data.
| return getNarrowSymbol(isoCode, errorCode); | ||
| case UCURR_FORMAL_SYMBOL_NAME: | ||
| return getFormalSymbol(isoCode, errorCode); | ||
| case UCURR_VARIANT_SYMBOL_NAME: |
There was a problem hiding this comment.
Did we intend to add the formal symbol here?
| return ((UnicodeString *)result)->getBuffer(); | ||
| } | ||
|
|
||
| FormattingData *CurrencyDisplayNames::fetchFormattingData(const UChar *isoCode, |
There was a problem hiding this comment.
Here and on the following two functions: who owns the return value?
| } else { | ||
| errorCode = ec2; | ||
| } | ||
| formattingDataCache = result; |
There was a problem hiding this comment.
Here and on the following two functions: this isn't thread-safe, and you need to delete the value previously in the static 1-element cache.
|
(Closing and opening the PR to trigger fresh CI) |
|
You currently have the following build error: That error usually means either you haven't defined the functions, or you haven't told the compiler where to find them. I think you're in the second case. You added "currencydisplaynames.cpp" to the vcxproj files, but you also need to add it to the file icu4c/source/common/sources.txt for Unixy systems. |
2 participants
Checklist