-
-
Notifications
You must be signed in to change notification settings - Fork 760
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-22991 Reduce Calendar object size #3327
Conversation
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: Java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. I second both of Markus's comments, but have none of my own to add.
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa! I guess I need to be careful what I ask for... I glanced at many of the changes, and they look plausible, and it looks like you are reducing the amount of work and reducing unused variables. I am stamping.
As for Java, I suspect that it might matter less there whether we store byte/short vs. int, but it might be nice to port the other changes there if they make sense. (Don't know what the Grego functions look like there since Java does not have output reference types.)
@richgillam ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at it again. LGTM.
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
I will deal with that in a separate PR later. |
Gregorian monthOfYear, dayOfMonth only need one byte dayOfYear only need two bytes to store.
These fields are used to store calculated value of day of year, day of month and month of year according to Gregorian calendar, and therefore will max to 365, 31 and 12 so it is a waste to store them with 4 bytes. We only need 9 bits, 5 bits, 4 bits for them. We can use bit fields to store but still need 18 bits which is more than 2 bytes. In this PR, I change to use 2 + 1 + 1 = 4 bytes to store them. Save 8 bytes per Calendar object
Checklist