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

ICU-23006 Fix Chinese Calendar getActualMaximize #3348

Merged
merged 1 commit into from
Jan 29, 2025
Merged

ICU-23006 Fix Chinese Calendar getActualMaximize #3348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions icu4c/source/i18n/chnsecal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,16 @@ int32_t ChineseCalendar::handleGetExtendedYear(UErrorCode& status) {
* @stable ICU 2.8
*/
int32_t ChineseCalendar::handleGetMonthLength(int32_t extendedYear, int32_t month, UErrorCode& status) const {
bool isLeapMonth = internalGet(UCAL_IS_LEAP_MONTH) == 1;
return handleGetMonthLengthWithLeap(extendedYear, month, isLeapMonth, status);
}

int32_t ChineseCalendar::handleGetMonthLengthWithLeap(int32_t extendedYear, int32_t month, bool leap, UErrorCode& status) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really worth it to split handleGetMonthLength() into two functions like this? Wouldn't it be easier to just make the change inside handleGetMonthLength()?

const Setting setting = getSetting(status);
if (U_FAILURE(status)) {
return 0;
}
int32_t thisStart = handleComputeMonthStart(extendedYear, month, true, status);
int32_t thisStart = handleComputeMonthStartWithLeap(extendedYear, month, leap, status);
if (U_FAILURE(status)) {
return 0;
}
Expand Down Expand Up @@ -333,6 +338,14 @@ struct MonthInfo computeMonthInfo(
* @stable ICU 2.8
*/
int64_t ChineseCalendar::handleComputeMonthStart(int32_t eyear, int32_t month, UBool useMonth, UErrorCode& status) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here: Is it really worth it to split handleComputeMonthStart() into two functions like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary because we cannot change the signature of handleComputeMonthStart since it is defined by the superclass but we need to pass in the leap information separately to avoid infinity recusion

bool isLeapMonth = false;
if (useMonth) {
isLeapMonth = internalGet(UCAL_IS_LEAP_MONTH) != 0;
}
return handleComputeMonthStartWithLeap(eyear, month, isLeapMonth, status);
}

int64_t ChineseCalendar::handleComputeMonthStartWithLeap(int32_t eyear, int32_t month, bool isLeapMonth, UErrorCode& status) const {
if (U_FAILURE(status)) {
return 0;
}
Expand Down Expand Up @@ -363,12 +376,6 @@ int64_t ChineseCalendar::handleComputeMonthStart(int32_t eyear, int32_t month, U
return 0;
}

// Ignore IS_LEAP_MONTH field if useMonth is false
bool isLeapMonth = false;
if (useMonth) {
isLeapMonth = internalGet(UCAL_IS_LEAP_MONTH) != 0;
}

int32_t newMonthYear = Grego::dayToYear(newMoon, status);

struct MonthInfo monthInfo = computeMonthInfo(setting, newMonthYear, newMoon, status);
Expand Down Expand Up @@ -1184,6 +1191,27 @@ ChineseCalendar::Setting ChineseCalendar::getSetting(UErrorCode&) const {
};
}

int32_t
ChineseCalendar::getActualMaximum(UCalendarDateFields field, UErrorCode& status) const
{
if (U_FAILURE(status)) {
return 0;
}
if (field == UCAL_DATE) {
LocalPointer<ChineseCalendar> cal(clone(), status);
if(U_FAILURE(status)) {
return 0;
}
cal->setLenient(true);
cal->prepareGetActual(field,false,status);
int32_t year = cal->get(UCAL_EXTENDED_YEAR, status);
int32_t month = cal->get(UCAL_MONTH, status);
bool leap = cal->get(UCAL_IS_LEAP_MONTH, status) != 0;
return handleGetMonthLengthWithLeap(year, month, leap, status);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is a partial answer to my question is here, but if you just left handleGetMonthLength() as one function and updated it to handle the leap year, couldn't you just call it here (it'd then be getting the UCAL_IS_LEAP_MONTH field instead of getting it here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is one usage need to use get (to force the calculation of all fields) and the other need to use internalGet (to avoid infinity recursion) and I cannot change the function signature (because it is defined in the base class as a protected method for all subclass of Calendar). The tricky part of this fix is it is very easy to get into infinity recursion

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Thanks for the explanation.

}
return Calendar::getActualMaximum(field, status);
}

U_NAMESPACE_END

#endif
Expand Down
6 changes: 6 additions & 0 deletions icu4c/source/i18n/chnsecal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,10 @@ class U_I18N_API ChineseCalendar : public Calendar {
virtual void handleComputeFields(int32_t julianDay, UErrorCode &status) override;
virtual const UFieldResolutionTable* getFieldResolutionTable() const override;

private:
int32_t handleGetMonthLengthWithLeap(int32_t extendedYear, int32_t month, bool isLeap, UErrorCode& status) const;
int64_t handleComputeMonthStartWithLeap(int32_t eyear, int32_t month, bool isLeap, UErrorCode& status) const;

public:
virtual void add(UCalendarDateFields field, int32_t amount, UErrorCode &status) override;
virtual void add(EDateFields field, int32_t amount, UErrorCode &status) override;
Expand Down Expand Up @@ -254,6 +258,8 @@ class U_I18N_API ChineseCalendar : public Calendar {
*/
virtual const char * getType() const override;

virtual int32_t getActualMaximum(UCalendarDateFields field, UErrorCode& status) const override;

struct Setting {
int32_t epochYear;
const TimeZone* zoneAstroCalc;
Expand Down
2 changes: 1 addition & 1 deletion icu4c/source/test/intltest/calregts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2433,7 +2433,7 @@ void CalendarRegressionTest::TestT5555()

// Should be set to Wednesday, February 24, 2010
if (U_FAILURE(ec) || yy != 2010 || mm != UCAL_FEBRUARY || dd != 24 || ee != UCAL_WEDNESDAY) {
errln("FAIL: got date %4d/%02d/%02d, expected 210/02/24: ", yy, mm + 1, dd);
errln("FAIL: got date %4d/%02d/%02d (wd=%d), expected 2010/02/24: ", yy, mm + 1, dd, ee);
}
delete cal;
}
Expand Down
25 changes: 25 additions & 0 deletions icu4c/source/test/intltest/caltest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ void CalendarTest::runIndexedTest( int32_t index, UBool exec, const char* &name,
TESTCASE_AUTO(TestCalendarRollOrdinalMonth);
TESTCASE_AUTO(TestLimitsOrdinalMonth);
TESTCASE_AUTO(TestActualLimitsOrdinalMonth);
TESTCASE_AUTO(TestMaxActualLimitsWithoutGet23006);
TESTCASE_AUTO(TestChineseCalendarMonthInSpecialYear);
TESTCASE_AUTO(TestClearMonth);

Expand Down Expand Up @@ -5354,6 +5355,30 @@ void CalendarTest::TestLimitsOrdinalMonth() {
}
}

void CalendarTest::TestMaxActualLimitsWithoutGet23006() {
UErrorCode status = U_ZERO_ERROR;
GregorianCalendar gc(status);
gc.set(2025, UCAL_AUGUST, 8);
LocalPointer<Calendar> cal(Calendar::createInstance(Locale("en@calendar=chinese"), status), status);
cal->setTime(gc.getTime(status), status);
int32_t beforeCallingGet = cal->getActualMaximum(UCAL_DAY_OF_MONTH, status);
cal->get(UCAL_DAY_OF_MONTH, status);
int32_t afterCallingGet = cal->getActualMaximum(UCAL_DAY_OF_MONTH, status);
assertEquals("getActualMaximum() should return same value before/after calling get()",
beforeCallingGet, afterCallingGet);
assertEquals("getActualMaximum() should return 29 before calling get()",
29, beforeCallingGet);

gc.set(2026, UCAL_AUGUST, 8);
cal->setTime(gc.getTime(status), status);
beforeCallingGet = cal->getActualMaximum(UCAL_DAY_OF_MONTH, status);
cal->get(UCAL_DAY_OF_MONTH, status);
afterCallingGet = cal->getActualMaximum(UCAL_DAY_OF_MONTH, status);
assertEquals("getActualMaximum() should return same value before/after calling get()",
beforeCallingGet, afterCallingGet);
assertEquals("getActualMaximum() should return 29 before calling get()",
30, beforeCallingGet);
}
FrankYFTang marked this conversation as resolved.
Show resolved Hide resolved
void CalendarTest::TestActualLimitsOrdinalMonth() {
UErrorCode status = U_ZERO_ERROR;
GregorianCalendar gc(status);
Expand Down
1 change: 1 addition & 0 deletions icu4c/source/test/intltest/caltest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ class CalendarTest: public CalendarTimeZoneTest {
void TestCalendarRollOrdinalMonth();
void TestLimitsOrdinalMonth();
void TestActualLimitsOrdinalMonth();
void TestMaxActualLimitsWithoutGet23006();
void TestDangiOverflowIsLeapMonthBetween22507();

void TestFWWithISO8601();
Expand Down
1 change: 1 addition & 0 deletions icu4j/main/core/src/main/java/com/ibm/icu/util/Calendar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6157,6 +6157,7 @@ abstract protected int handleComputeMonthStart(int eyear, int month,
* @stable ICU 2.0
*/
protected int handleGetMonthLength(int extendedYear, int month) {
System.out.printf("handleGetMonthLength(ey=%d, m=%d)", extendedYear, month);
return handleComputeMonthStart(extendedYear, month+1, true) -
handleComputeMonthStart(extendedYear, month, true);
}
Expand Down
33 changes: 29 additions & 4 deletions icu4j/main/core/src/main/java/com/ibm/icu/util/ChineseCalendar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,12 @@ protected int handleGetExtendedYear() {
* @stable ICU 2.8
FrankYFTang marked this conversation as resolved.
Show resolved Hide resolved
*/
protected int handleGetMonthLength(int extendedYear, int month) {
int thisStart = handleComputeMonthStart(extendedYear, month, true) -
int isLeapMonth = internalGet(IS_LEAP_MONTH);
return handleGetMonthLengthWithLeap(extendedYear, month, isLeapMonth);
}

private int handleGetMonthLengthWithLeap(int extendedYear, int month, int isLeap) {
int thisStart = handleComputeMonthStartWithLeap(extendedYear, month, isLeap) -
EPOCH_JULIAN_DAY + 1; // Julian day -> local days
int nextStart = newMoonNear(thisStart + SYNODIC_GAP, true);
return nextStart - thisStart;
Expand Down Expand Up @@ -971,6 +976,14 @@ private int newYear(int gyear) {
* @stable ICU 2.8
*/
protected int handleComputeMonthStart(int eyear, int month, boolean useMonth) {
int isLeapMonth = 0;
if (useMonth) {
isLeapMonth = internalGet(IS_LEAP_MONTH);
}
return handleComputeMonthStartWithLeap(eyear, month, isLeapMonth);
}

private int handleComputeMonthStartWithLeap(int eyear, int month, int isLeapMonth) {

// If the month is out of range, adjust it into range, and
// modify the extended year value accordingly.
Expand All @@ -991,9 +1004,6 @@ protected int handleComputeMonthStart(int eyear, int month, boolean useMonth) {
int saveOrdinalMonth = internalGet(ORDINAL_MONTH);
int saveIsLeapMonth = internalGet(IS_LEAP_MONTH);

// Ignore IS_LEAP_MONTH field if useMonth is false
int isLeapMonth = useMonth ? saveIsLeapMonth : 0;

computeGregorianFields(julianDay);

// This will modify the MONTH and IS_LEAP_MONTH fields (only)
Expand Down Expand Up @@ -1161,6 +1171,21 @@ protected int internalGetMonth(int defaultValue)
return internalGetMonth();
}

public int getActualMaximum(int field) {
if (field == DAY_OF_MONTH) {
Calendar cal = (Calendar) clone();
cal.setLenient(true);
cal.prepareGetActual(field, false);
int eyear = cal.get(EXTENDED_YEAR);
int month = cal.get(MONTH);
int isLeap = cal.get(IS_LEAP_MONTH);

return handleGetMonthLengthWithLeap(eyear, month, isLeap);
}
return super.getActualMaximum(field);

}

/*
private static CalendarFactory factory;
public static CalendarFactory factory() {
Expand Down
28 changes: 28 additions & 0 deletions icu4j/main/core/src/test/java/com/ibm/icu/dev/test/calendar/CalendarRegressionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2718,5 +2718,33 @@ public void TestIslamicUmalquraCalendarSlow() { // ICU-22513

}

@Test
public void TestMaxActualLimitsWithoutGet23006() {
Calendar calendar = Calendar.getInstance(new Locale("zh_zh@calendar=chinese"));
// set day equal to 8th August 2025 in Gregorian calendar
// this is a leap month in Chinese calendar
GregorianCalendar gc = new GregorianCalendar(TimeZone.GMT_ZONE);
gc.clear();
gc.set(2025, Calendar.AUGUST, 8);
calendar.setTimeInMillis(gc.getTimeInMillis());
int actualMaximumBeforeCallingGet = calendar.getActualMaximum(Calendar.DAY_OF_MONTH);
assertTrue("get(ERA)", calendar.get(Calendar.ERA) > 0); // calling get will cause to compute fields
int actualMaximumAfterCallingGet = calendar.getActualMaximum(Calendar.DAY_OF_MONTH);
assertEquals("calling getActualMaximum before/after calling get should be the same",
actualMaximumBeforeCallingGet, actualMaximumAfterCallingGet);
assertEquals("calling getActualMaximum before should return 29",
29, actualMaximumBeforeCallingGet);

gc.set(2026, Calendar.AUGUST, 8);
calendar.setTimeInMillis(gc.getTimeInMillis());
actualMaximumBeforeCallingGet = calendar.getActualMaximum(Calendar.DAY_OF_MONTH);
assertTrue("get(ERA)", calendar.get(Calendar.ERA) > 0); // calling get will cause to compute fields
actualMaximumAfterCallingGet = calendar.getActualMaximum(Calendar.DAY_OF_MONTH);
assertEquals("calling getActualMaximum before/after calling get should be the same",
actualMaximumBeforeCallingGet, actualMaximumAfterCallingGet);
assertEquals("calling getActualMaximum before should return 30",
30, actualMaximumBeforeCallingGet);

}
}
//eof
Loading