ICU-23006 Fix Chinese Calendar getActualMaximize #3348
Conversation
FrankYFTang
changed the title
There was a problem hiding this comment.
Looks fine, I think, and I think I'm okay with you checking in as-is, so I went ahead and approved, but I do have some questions you might want to consider. Most of those questions apply to both the C++ and Java versions of your change, but I didn't repeat them on the Java code.
| return handleGetMonthLengthWithLeap(extendedYear, month, isLeapMonth, status); | ||
| } | ||
|
|
||
| int32_t ChineseCalendar::handleGetMonthLengthWithLeap(int32_t extendedYear, int32_t month, bool leap, UErrorCode& status) const { |
There was a problem hiding this comment.
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()?
| @@ -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 { | |||
There was a problem hiding this comment.
Same question here: Is it really worth it to split handleComputeMonthStart() into two functions like this?
There was a problem hiding this comment.
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
| 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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Okay. Thanks for the explanation.
|
rslgtm, leaving it to you two -- tnx! |
|
PTAL |
FrankYFTang
force-pushed
the
ICU-23006-GetActual
branch
from
df69c12 to
a6685a9
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
|
tests pass -- ready to merge? |
Read the IS_LEAP_MONTH field correctly while calling getActualMaximize.
Chinese Calendar need to read additional IS_LEAP_MONTH field while calculate the start and length of month, make the value read from the out most calls and passed into function parameter.
Checklist