-
-
Notifications
You must be signed in to change notification settings - Fork 763
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-22559 Hardcode the macroregions and add a debug assertion #2688
Conversation
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.
Thanks for working on this!
Unfortunately, there are test failures. For one of them, we will need to cherry-pick PR #2685 from the maint/maint-74 branch to main. Or maybe just include the one-line icu4c/source/test/depstest/dependencies.txt change here as well? FYI @echeran
For the warnings-as-errors failures, see below.
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.
lgtm tnx -- pse squash
063a23b
to
1c4df7b
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Can I merge despite the following depstest error which seems unrelated?
|
I think that's ok. That failure will be fixed by the maintenance branch change that I pointed out, and that you have in your merger back to main. |
@@ -352,7 +352,53 @@ UBool U_CALLCONV cleanup() { | |||
return true; | |||
} | |||
|
|||
static const char16_t* MACROREGION_HARDCODE[] = { |
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.
Could you add a comment pointing to the
https://github.com/unicode-org/icu/blob/main/icu4c/source/data/misc/supplementalData.txt#L7661
... region:macroregion{
This is the first working prototype. Possible future work (unclear if we should block this PR):
Checklist