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 clean up error handling in PR#3230 #3231

Merged
merged 1 commit into from
Feb 13, 2025
Merged

ICU-22934 clean up error handling in PR#3230 #3231

merged 1 commit into from
Feb 13, 2025

Conversation

FrankYFTang
Copy link
Contributor

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 FrankYFTang requested a review from aheninger October 2, 2024 23:43
@aheninger
Copy link
Contributor

Please add a unit test that triggers the recursion depth failures. It's not immediately obvious that there won't be memory leaks or other problems when they occur.

In functions with a newly added UErrorCode in PR #3230, double check the error handling. RBBINode::cloneTree(), for instance,

  • at line 216, set a U_MEMORY_ALLOCATION_ERROR and return immediately if the new fails.
  • make sure that you return a nullptr right away, and don't leak, in the event of a failure returned from the recursive calls at lines 220 and 226

In RBBINode::flattenSets(), maybe make the setRefNode variables into localPointers, so you can more easily return immediately if the cloneTree() calls set an error.

@FrankYFTang FrankYFTang force-pushed the ICU-22934-rbbi-limit2 branch from e86eab7 to 2d3da3e Compare October 10, 2024 01:50
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/rbbinode.cpp is different
  • icu4c/source/common/rbbiscan.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

@FrankYFTang FrankYFTang changed the title ICU-22934 Fix typo in PR#3230 ICU-22934 clean up error handling in PR#3230 Oct 10, 2024
@FrankYFTang FrankYFTang force-pushed the ICU-22934-rbbi-limit2 branch from 2d3da3e to 5cbbcef Compare October 10, 2024 02:23
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/rbbinode.cpp is different
  • icu4c/source/common/rbbiscan.cpp is different
  • icu4c/source/common/rbbisetb.cpp is different
  • icu4c/source/common/rbbitblb.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@aheninger
Copy link
Contributor

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

Hi Frank,
I've started to look at creating test cases, but have run into some behavior that I don't fully understand. I'll continue with it and get you something workable. Unfortunately I will be busy for most of the coming week, so it won't happen right away.

@FrankYFTang
Copy link
Contributor Author

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

Hi Frank, I've started to look at creating test cases, but have run into some behavior that I don't fully understand. I'll continue with it and get you something workable. Unfortunately I will be busy for most of the coming week, so it won't happen right away.

ping

@FrankYFTang
Copy link
Contributor Author

ping

1 similar comment
@FrankYFTang
Copy link
Contributor Author

ping

@richgillam richgillam self-assigned this Jan 16, 2025
@richgillam
Copy link
Contributor

Well, looks like @aheninger is not responding. Either I can take a look or @eggrobin can...

@markusicu
Copy link
Member

@richgillam:

Well, looks like @aheninger is not responding. Either I can take a look or @eggrobin can...

The ticket already has another merged PR, so the ticket must be closed by Wed feb12.
Please work with @FrankYFTang to either land this PR here or retarget it to a new ticket.

Note that aside from Andy's comments there are also CI check failures.

@FrankYFTang
ICU-22934 Fix typo in PR#3230
9dfa887 
ICU-22934 Fix error handling while new return nullptr

ICU-22934 Simplfied

ICU-22934 more

ICU-22934 fix leak
@FrankYFTang FrankYFTang force-pushed the ICU-22934-rbbi-limit2 branch from 5cbbcef to 9dfa887 Compare February 12, 2025 23:48
@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

@FrankYFTang
Copy link
Contributor Author

rebase to figut out what is the other breakage on the CI first.

@FrankYFTang FrankYFTang removed the request for review from aheninger February 12, 2025 23:51
@FrankYFTang
Copy link
Contributor Author

@eggrobin @richgillam could you review this again. I try to fix the CI failure if they show up. @aheninger said he will add more tests but not coming back to shis for along time. How about we land this after I fix all CI issues

richgillam
richgillam approved these changes Feb 12, 2025
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.

LGTM

@richgillam
Copy link
Contributor

@eggrobin @richgillam could you review this again.

The changes look fine to me.

I try to fix the CI failure if they show up. @aheninger said he will add more tests but not coming back to shis for along time. How about we land this after I fix all CI issues

I'm fine with landing this in its current state and filing a ticket or something for the additional tests Andy wants to see.

@FrankYFTang FrankYFTang merged commit 59719f0 into unicode-org:main Feb 13, 2025
94 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22934-rbbi-limit2 branch February 13, 2025 00:56
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

@eggrobin eggrobin Awaiting requested review from eggrobin

Assignees

@richgillam richgillam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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