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

Jump to bottom

ICU-20046 Fix some OOM issues in the RBNF class. #24

Merged
merged 2 commits into from
Aug 3, 2018
Merged

ICU-20046 Fix some OOM issues in the RBNF class. #24

merged 2 commits into from
Aug 3, 2018

Conversation

jefgen
Copy link
Member

@jefgen jefgen commented Jul 24, 2018

There are a few locations in the RuleBasedNumberFormat class that don't check for out-of-memory (OOM) failures.

aheninger
aheninger reviewed Jul 24, 2018
View reviewed changes
icu4c/source/i18n/rbnf.cpp
@@ -1133,8 +1133,17 @@ RuleBasedNumberFormat::format(const DecimalQuantity &number,

// TODO this section should probably be optimized. The DecimalFormat is shared in ICU4J.
NumberFormat *decimalFormat = NumberFormat::createInstance(locale, UNUM_DECIMAL, status);
if (decimalFormat == nullptr) {
Copy link
Contributor

@aheninger aheninger Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It would be cleaner to use LocalPointer<NumberFormat> decimialFormat(...
It gets rid of the delete decimalformat; lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking a look at the changes Andy! :)
I've pushed a new commit that uses LocalPointer instead, which cleans up the manual delete lines.

aheninger
aheninger reviewed Jul 24, 2018
View reviewed changes
icu4c/source/i18n/rbnf.cpp
@@ -1166,8 +1175,17 @@ RuleBasedNumberFormat::format(const DecimalQuantity &number,

// TODO this section should probably be optimized. The DecimalFormat is shared in ICU4J.
NumberFormat *decimalFormat = NumberFormat::createInstance(locale, UNUM_DECIMAL, status);
if (decimalFormat == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, LocalPointer<NumberFormat> would be simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Andy! This should be resolved in the latest commit.

aheninger
aheninger reviewed Jul 24, 2018
View reviewed changes
Copy link
Contributor

@aheninger aheninger left a comment

Choose a reason for hiding this comment

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

LGTM. LocalPointer could simplify a bit by getting rid of some explicit deletes.

@jefgen
Copy link
Member Author

jefgen commented Aug 2, 2018

Setting Andy as the primary reviewer (aka assignee), since he has already provided great feedback and commented LGTM. (Though GitHub doesn't consider a comment the same as an actual review.)

aheninger
aheninger approved these changes Aug 3, 2018
View reviewed changes
Copy link
Contributor

@aheninger aheninger left a comment

Choose a reason for hiding this comment

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

LGTM

@jefgen jefgen merged commit b6dd315 into unicode-org:master Aug 3, 2018
@jefgen jefgen deleted the jefgen/20046 branch August 3, 2018 20:30
sffc referenced this pull request in sffc/icu Sep 26, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
e123dd1 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
sffc referenced this pull request in sffc/icu Sep 26, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
2c0ea26 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
sffc referenced this pull request in sffc/icu Sep 27, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
c472cff 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
sffc referenced this pull request in sffc/icu Sep 27, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
2ad79a1 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
sffc referenced this pull request in sffc/icu Sep 27, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
e03ecbc 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
sffc referenced this pull request in sffc/icu Sep 27, 2018
@jefgen @sffc
ICU-20046 Improve OOM error checking in the RBNF class. (#24)
c9573ab 
- There are a few locations in the RBNF class that don't check for out-of-memory (OOM) failures.
- Using LocalPointer to clean up the manual deletes.
- Change to use nullptr instead of NULL.
- A few minor typo fixes as well.
hugovdm pushed a commit to hugovdm/icu that referenced this pull request Apr 29, 2020
@younies
Merge pull request unicode-org#24 from younies/units_router_younies
5936a77 
Units router younies
ctrlaltf24 pushed a commit to FaithLife-Community/icu that referenced this pull request Oct 23, 2024
@jefgen
Fix the naming of the dependent runtime Nuget packages. (unicode-org#24)
38f7b5d 
It turns out that Nuget.org won't accept any packages that start with a `runtime*` prefix.
So we'll need to move the Runtime ID (RID) to the end of the Nuget package name for
the dependent runtime packages instead.

This change also bumps the version number, as Nuget.org won't let you re-submit any
package with the same version once it has been removed/deleted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aheninger aheninger aheninger approved these changes

@srl295 srl295 Awaiting requested review from srl295

@sffc sffc Awaiting requested review from sffc

@markusicu markusicu Awaiting requested review from markusicu

@axelandrejs axelandrejs Awaiting requested review from axelandrejs

@mingyzha mingyzha Awaiting requested review from mingyzha

@daniel-ju daniel-ju Awaiting requested review from daniel-ju

Assignees

@aheninger aheninger

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jefgen @aheninger