-
-
Notifications
You must be signed in to change notification settings - Fork 754
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-21957 integrate CLDR release-42-m1 (early milestone) to ICU main for ICU 72 #2103
ICU-21957 integrate CLDR release-42-m1 (early milestone) to ICU main for ICU 72 #2103
Conversation
c58675c
to
adfd3bc
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
/azp run CI-Exhaustive |
Azure Pipelines successfully started running 1 pipeline(s). |
52aef36
to
514d9ad
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
/azp run CI-Exhaustive |
Azure Pipelines successfully started running 1 pipeline(s). |
3398dd9
to
fa702a4
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
/azp run CI-Exhaustive |
Azure Pipelines successfully started running 1 pipeline(s). |
Review tip: $ gh pr checkout 2103
$ git diff fa702a450462925d520dc86a372c77827e82aa38..e17219582ed0396d993927066e95af0c9199f8db -IcldrVersion -IVersion (ignore version-number changes) |
icu4c/source/i18n/smpdtfmt.cpp
Outdated
static const UChar gDefaultPattern[] = | ||
{ | ||
0x79, 0x79, 0x79, 0x79, 0x4D, 0x4D, 0x64, 0x64, 0x20, 0x68, 0x68, 0x3A, 0x6D, 0x6D, 0x20, 0x61, 0 | ||
}; /* "yyyyMMdd hh:mm a" */ | ||
0x79, 0x4D, 0x4D, 0x64, 0x64, 0x20, 0x68, 0x68, 0x3A, 0x6D, 0x6D, 0x202F, 0x61, 0 | ||
}; /* "yMMdd hh:mm\u202Fa" */ |
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.
note that the gDefaultPattern changed.
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.
Yes. We had changed yyyy -> y in CLDR some years ago, so updating per that. I also added the \u202F here for consistency with the new formats. Not completely sure about that.
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.
@srl295 I decided to revert the use of \u202F in gDefaultPattern, so now the only thing that has changed is yyyy -> y.
27cdd35
to
9cefa0f
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
/azp run CI-Exhaustive |
Azure Pipelines successfully started running 1 pipeline(s). |
…for 72 (rebased on main) + FormattedStringBuilderTest::testInsertOverflow infolns,logKnownIssue skip for CI exhaustive crash
8b9ddc8
to
edfbe17
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
/azp run CI-Exhaustive |
Azure Pipelines successfully started running 1 pipeline(s). |
@srl295 and @macchiati, for an exhaustive test failure (nothing to do with this PR) I had to add some logging and a logKnownIssue skip in FormattedStringBuilderTest::testInsertOverflow(). Also I reverted part of the change to gDefaultPattern in ICU4C SimpleDateFormat, so the only change there is now just yyyy -> y. Other that those two things, this PR is the same as what you previously approved, but needs re-approval. Thanks! |
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.
diff lgtm
Hmm, C: Linux Clang Exhaustive Tests (Ubuntu 18.04) still crashing in FormattedStringBuilderTest:testInsertOverflow(), will submit a separate PR with a fix for that. |
The PR for that is #2104 But the crash itself may be related to the following change in FormattedStringBuilder per https://unicode-org.atlassian.net/browse/ICU-22005 (Integer overflow leading to OOB/CHECK in icu_71::FormattedStringBuilder::prepareForInsertHelper): PR #2070 |
* tests/Rcmail/Rcmail.php (test_format_date): Starting with ICU 72.1, a NARROW NO-BREAK SPACE (NNBSP) is used instead of an ASCII space before the meridian. So, check for an NNBSP when using ICU >= 72.1. References: * https://icu.unicode.org/download/72 * unicode-org/icu#2103
* tests/Rcmail/Rcmail.php (test_format_date): Starting with ICU 72.1, a NARROW NO-BREAK SPACE (NNBSP) is used instead of an ASCII space before the meridian. So, check for an NNBSP when using ICU >= 72.1. References: * https://icu.unicode.org/download/72 * https://cldr.unicode.org/index/downloads/cldr-42 * unicode-org/icu#2103
* tests/Rcmail/Rcmail.php (test_format_date): Starting with ICU 72.1, a NARROW NO-BREAK SPACE (NNBSP) is used instead of an ASCII space before the meridian. So, check for an NNBSP when using ICU >= 72.1. References: * https://icu.unicode.org/download/72 * https://cldr.unicode.org/index/downloads/cldr-42 * unicode-org/icu#2103
* tests/Rcmail/Rcmail.php (test_format_date): Starting with ICU 72.1, a NARROW NO-BREAK SPACE (NNBSP) is used instead of an ASCII space before the meridian. So, check for an NNBSP when using ICU >= 72.1. References: * https://icu.unicode.org/download/72 * https://cldr.unicode.org/index/downloads/cldr-42 * unicode-org/icu#2103
Checklist
This integrates CLDR release-42-m1 (milestone release as of the start of general submission) to ICU. Several significant changes:
Note: I added a logKnownIssue skip for part of FormattedStringBuilderTest::testInsertOverflow() to address a crash that only happens running Ci exhaustive tests for Clang/Linux (not on my local system) which appears to have nothing to do with this PR; see https://unicode-org.atlassian.net/browse/ICU-22047. When the problematic code is skipped, the test uses an alternative way of generating a long enough UnicodeString so the remaining tests in that function are valid.