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-22642 Avoid spending too much time inside CanonicalIterator #3017

Merged
merged 1 commit into from
Jun 6, 2024
Merged

ICU-22642 Avoid spending too much time inside CanonicalIterator #3017

merged 1 commit into from
Jun 6, 2024

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented May 30, 2024 •
edited
Loading

Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65247

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22642
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • 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.
  • 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 FrankYFTang requested review from markusicu, roubert and richgillam and removed request for roubert May 30, 2024 01:13
richgillam
richgillam previously approved these changes May 30, 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.

Seems reasonable to me. I assume you're depending on Markus to help with a test case?

@FrankYFTang
Copy link
Contributor Author

Seems reasonable to me. I assume you're depending on Markus to help with a test case?

The test case is inside https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65247

@FrankYFTang FrankYFTang force-pushed the ICU-22642-limit-size branch from bdd784c to 7ccad53 Compare May 30, 2024 02:09
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/canittst.cpp is now changed in the branch
  • icu4c/source/test/intltest/canittst.h is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

I added a test to show the problem, without the fix, the constructor will run for a very long time. (several minutes)

roubert
roubert requested changes May 30, 2024
View reviewed changes
icu4c/source/common/caniter.cpp Outdated Show resolved Hide resolved
icu4c/source/common/caniter.cpp Show resolved Hide resolved
icu4c/source/test/intltest/canittst.cpp Outdated Show resolved Hide resolved
@FrankYFTang FrankYFTang force-pushed the ICU-22642-limit-size branch from 7ccad53 to a557299 Compare May 30, 2024 18:29
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/caniter.cpp is different
  • icu4j/main/core/src/main/java/com/ibm/icu/text/CanonicalIterator.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/normalizer/TestCanonicalIterator.java is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang requested review from roubert and richgillam May 30, 2024 23:19
@FrankYFTang FrankYFTang force-pushed the ICU-22642-limit-size branch 2 times, most recently from a557299 to 400a13d Compare June 1, 2024 10:02
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/canittst.cpp is different
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/normalizer/TestCanonicalIterator.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam
richgillam previously approved these changes Jun 3, 2024
View reviewed changes
@FrankYFTang FrankYFTang force-pushed the ICU-22642-limit-size branch from 400a13d to 6b65512 Compare June 3, 2024 22:20
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/canittst.cpp is no longer changed in the branch
  • icu4c/source/test/intltest/canittst.h is no longer changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/normalizer/TestCanonicalIterator.java is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang requested a review from richgillam June 3, 2024 22:20
@FrankYFTang FrankYFTang requested a review from roubert June 5, 2024 18:28
@FrankYFTang
Copy link
Contributor Author

PTAL

richgillam
richgillam approved these changes Jun 5, 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.

Still LGTM.

@FrankYFTang FrankYFTang force-pushed the ICU-22642-limit-size branch from 9947e91 to 591c31c Compare June 6, 2024 00:22
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

roubert
roubert approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@roubert roubert left a comment

Choose a reason for hiding this comment

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

LGTM

@roubert roubert merged commit 87fce24 into unicode-org:main Jun 6, 2024
103 checks passed
echeran pushed a commit to echeran/icu that referenced this pull request Jun 28, 2024
@FrankYFTang @echeran
ICU-22642 Avoid spending too much time inside CanonicalIterator
fd67633 
See unicode-org#3017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richgillam richgillam richgillam approved these changes

@roubert roubert roubert approved these changes

@markusicu markusicu Awaiting requested review from markusicu

Assignees

@richgillam richgillam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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