-
-
Notifications
You must be signed in to change notification settings - Fork 744
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.
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-22950 Do not initialize currency for NumberPropertyMapper when useCurrency is false #3250
base: main
Are you sure you want to change the base?
ICU-22950 Do not initialize currency for NumberPropertyMapper when useCurrency is false #3250
Conversation
resolveCurrency() slows down simple number formatting requests like: for (int i = 0; i < 9999; i++) u_snprintf_u(buff, 100, u"%d", i); Since we don't use currency here, we can skip it's initialization.
CurrencyUnit currency; | ||
UCurrencyUsage currencyUsage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expanded the rest of this (long) function, searched for "currency", and found some code that uses these variables but is gated by conditions other than useCurrency
.
if (!properties.currencyUsage.isNull())
if (exportedProperties != nullptr)
If we think we can guarantee that useCurrency is true there, then we should add U_ASSERT(useCurrency)
there.
For the second one, maybe a default-constructed currency
is fine if not useCurrency
?
Otherwise, we may need to add a runtime check for useCurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markus is right; this is a concern.
For if (!properties.currencyUsage.isNull())
, I wonder if this could just be changed to useCurrency
?
For exportedProperties->currency
, I agree that a default-constructed currency
ought to be fine.
For if (precision.fType == Precision::PrecisionType::RND_CURRENCY)
, I wonder if we could change this to if (useCurrency && precision.fType == Precision::PrecisionType::RND_CURRENCY)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assertion to if (!properties.currencyUsage.isNull())
, since useCurrency is true when !properties.currencyUsage.isNull()
Used if (useCurrency && precision.fType == Precision::PrecisionType::RND_CURRENCY)
, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for the changes!
resolveCurrency() slows down simple number formatting requests like:
Since icu4c doesn't use currency there, we can skip its initialization.
Checklist