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-22934 Limit the number of resursive call #3230

Merged
merged 1 commit into from
Oct 2, 2024
Merged

ICU-22934 Limit the number of resursive call #3230

merged 1 commit into from
Oct 2, 2024

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Oct 1, 2024 •
edited
Loading

To avoid stack overflow while building RBBI

Checklist

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22934
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang
ICU-22934 Limit the number of resursive call
440b39b 
To avoid stack overflow while building RBBI
richgillam
richgillam approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Looks plausible to me, although I'll defer to Andy...

@FrankYFTang FrankYFTang merged commit 5b45e5c into unicode-org:main Oct 2, 2024
94 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22934-rbbi-limit branch October 2, 2024 21:02
aheninger
aheninger reviewed Oct 2, 2024
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.

I would like to see a test added for this. I know the rule string will be very long, but it will mostly be repeated fragments that can be assembled in a small loop.

icu4c/source/common/rbbinode.cpp
fRightChild->fParent = this;
fRightChild = replTree->cloneTree(status, depth+1);
if (U_FAILURE(status)) {
fRightChild->fParent = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong. Did you intend to return on failure?

@aheninger aheninger mentioned this pull request Oct 3, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aheninger aheninger aheninger left review comments

@richgillam richgillam richgillam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrankYFTang @aheninger @richgillam