Skip to content

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Aug 27, 2025

Remove unnecessary structure to support other epoch in Chinese and Dangi calendar. Rename and keep CYCLE_EPOCH for the calculation of 60 years cycle.
This should only change the meaning of values for set/get extended year and woy year.
Also need to address some int32_t overflow issue after the value of gyear shift

The extended year value of "chinese" calendar reduced 2637 (newValue = oldValue -2637), and the value of "dangi" calendar reduced 2333 (newValue = oldValue -2333).

Checklist

  • Required: Issue filed: ICU-23167
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • 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 sffc August 27, 2025 01:29
@FrankYFTang FrankYFTang force-pushed the ICU-23167-chinese-epoch branch from 6a7a7d5 to ff13389 Compare August 27, 2025 18:16
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/chnsecal.cpp is different
  • icu4j/main/core/src/main/java/com/ibm/icu/util/ChineseCalendar.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

Notice during the change, I also discover an unrelated minor issue ICU-23198. That issue is not addressed in this PR but will handle separately

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Aug 28, 2025
@FrankYFTang FrankYFTang force-pushed the ICU-23167-chinese-epoch branch from 543b395 to e2480e3 Compare August 28, 2025 01:04
@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

Remove unnecessary structure to support other epoch in
Chinese and Dangi calendar. Rename and keep CYCLE_EPOCH
for the calculation of 60 years cycle.
This should only change the meaning of values for set/get extended year
and woy year.
@FrankYFTang FrankYFTang force-pushed the ICU-23167-chinese-epoch branch from e2480e3 to 06e09ef Compare August 28, 2025 20:09
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/caltest.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

@sffc ping

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Nice work!

What is calendar.res and why did it change in this PR?

@@ -253,7 +247,6 @@ class U_I18N_API ChineseCalendar : public Calendar {
virtual int32_t getActualMaximum(UCalendarDateFields field, UErrorCode& status) const override;

struct Setting {
int32_t epochYear;
Copy link
Member

Choose a reason for hiding this comment

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

Praise: Clever to use the mechanism already in place to make Chinese and Dangi have different extended years in order to change the extended year for both calendars.

if (info.month < 10 ||
int extended_year = gyear - CHINESE_EPOCH_YEAR;
int cycle_year = gyear - CYCLE_EPOCH;
if (info.month < 10 || // TODO(ICU-23198) < 10 or < 11 ????
Copy link
Member

Choose a reason for hiding this comment

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

I think 10 is correct; it just needs to be a month that is always after July and before December. So I think anything between 8 and 11 should work. The condition should return false if the Chinese date is before the Gregorian new year (most of the year) and true if it is after the Gregorian new year (the last 3-8 weeks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants