-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[SR-6303] Handle non-existing UCalendarDateFields without crashing #1298
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
[SR-6303] Handle non-existing UCalendarDateFields without crashing #1298
Conversation
| XCTAssertEqual(recreatedComponents.minute, 38) | ||
|
|
||
| // Nanoseconds are currently not supported by UCalendar C API, returns nil | ||
| // XCTAssertEqual(recreatedComponents.nanosecond, 40) |
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.
Is this a function of which version of ICU we're linking against?
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.
@parkera it seems that 59.1 but even in the last one (60.1) there are no UCalendarDateFields for nanoseconds and quarter. The list of all available UCalendarDateFields of 60.1:
enum UCalendarDateFields {
UCAL_ERA,
UCAL_YEAR,
UCAL_MONTH,
UCAL_WEEK_OF_YEAR,
UCAL_WEEK_OF_MONTH,
UCAL_DATE,
UCAL_DAY_OF_YEAR,
UCAL_DAY_OF_WEEK,
UCAL_DAY_OF_WEEK_IN_MONTH,
UCAL_AM_PM,
UCAL_HOUR,
UCAL_HOUR_OF_DAY,
UCAL_MINUTE,
UCAL_SECOND,
UCAL_MILLISECOND,
UCAL_ZONE_OFFSET,
UCAL_DST_OFFSET,
UCAL_YEAR_WOY,
UCAL_DOW_LOCAL,
UCAL_EXTENDED_YEAR,
UCAL_JULIAN_DAY,
UCAL_MILLISECONDS_IN_DAY,
UCAL_IS_LEAP_MONTH,
/* Do not conditionalize the following with #ifndef U_HIDE_DEPRECATED_API,
* it is needed for layout of Calendar, DateFormat, and other objects */
UCAL_FIELD_COUNT,
UCAL_DAY_OF_MONTH=UCAL_DATE
};
I'm sorry if I understood something wrong.
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.
I think you're correct, the quarter implementation in ICU has been a problem for some time.
| var vector = [Int32]() | ||
| var compDesc = [Int8]() | ||
| _convert(comps.era, type: "E", vector: &vector, compDesc: &compDesc) | ||
| _convert(comps.era, type: "G", vector: &vector, compDesc: &compDesc) |
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.
Good catch on this one
|
@swift-ci please test |
|
@swift-ci please test and merge |
|
There have been conflicts added since we tried to merge this last time (maybe because of the drop by @phausler recently). Can you rebase your original change onto master please and resubmit? |
Also, one typo for the “era” components type was found.
|
@alblue I checked and the issue still persists after the High Sierra changes applied, so I rebased the changes. Thank you for reminding! |
|
@swift-ci please test and merge |
Resolves: SR-6303
Running this code:
causes Thread 1: EXC_BAD_ACCESS (code=1, address=0x608a12c5cd78)
in: CFCalendar._CFCalendarComposeAbsoluteTimeV()
on the line: ucal_set(calendar->_cal, field, value);
After investigating the issue, I found out that -1 is returned for UCalendarDateFields for nanoseconds and quarter calendar components. Also, one typo for the “era” components type was found.
I tried to handle this issue without crashing by moving -1 one level higher and handling this case so nil would be assigned to the nanoseconds and quarter fields instead of crashing.
Please let me know what you think.