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.

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-22513 Return error if days is too large in IslamicUmalquraCalendar #2680

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

FrankYFTang
Copy link
Contributor

The computation in IslamicUmalquraCalendar is slower than others, if the days is more than 10,000 years from the epoch, we just return error to avoid hang.

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22513
  • 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 added the incomplete Needs work; do not approve/merge as is. label Oct 25, 2023
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/calendar.cpp is now changed in the branch
  • icu4c/source/i18n/gregocal.cpp is now changed in the branch
  • icu4c/source/i18n/unicode/calendar.h is now changed in the branch
  • icu4c/source/i18n/unicode/gregocal.h is now changed in the branch
  • icu4j/main/core/src/main/java/com/ibm/icu/util/Calendar.java is now changed in the branch
  • icu4j/main/core/src/test/java/com/ibm/icu/dev/test/calendar/CalendarRegressionTest.java is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

If the year is too large it may overflow the int32_t variable and cause
slow or infinity loop, return error if the year is too large that the
conversion to day may overflow int32_t. Limit the value to max value of
int32_t divide by 400.
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/islamcal.cpp is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

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.

Makes sense to me, but one question about the Java unit test.

cal.clear();
cal.add(Calendar.YEAR, 1229080905);
cal.roll(Calendar.WEEK_OF_MONTH, 1499050699);
cal.fieldDifference(new Date(0), Calendar.YEAR_WOY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but shouldn't there be some kind of "expect exception to be thrown" logic in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Java use long (64 bits) for those value so it will not throw exception with that setting. The test just make sure it will not be infinity loop or time out.

@FrankYFTang FrankYFTang merged commit cdab88f into unicode-org:main Oct 27, 2023
99 of 100 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22513-calendar-slow branch October 27, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incomplete Needs work; do not approve/merge as is.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants