Skip to content
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

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Oct 27, 2023

This is the first working prototype. Possible future work (unclear if we should block this PR):

  1. Make sure that there is a test that fails when macroregions aren't being loaded (removing them does not seem to cause any test failures in intltest, which is concerning)
  2. Convert the data-consistency assertion to a unit test
  3. Port this change to Java
Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22559
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@sffc sffc requested a review from markusicu October 27, 2023 01:55
Copy link
Member

@markusicu markusicu left a 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.

icu4c/source/common/loclikelysubtags.cpp Outdated Show resolved Hide resolved
icu4c/source/common/loclikelysubtags.cpp Show resolved Hide resolved
@sffc
Copy link
Member Author

sffc commented Oct 27, 2023

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

#2689

@sffc sffc changed the title ICU-22559 Hardcode the macroregions and add a test ICU-22559 Hardcode the macroregions and add a debug assertion Oct 27, 2023
Copy link
Member

@markusicu markusicu left a 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

@sffc sffc force-pushed the macroregions-hardcode branch from 063a23b to 1c4df7b Compare October 27, 2023 19:33
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member Author

sffc commented Oct 27, 2023

Can I merge despite the following depstest error which seems unrelated?

Error: in group ustream: io/ustream.o imports std::ios_base_library_init() - is this a new system symbol?

@markusicu
Copy link
Member

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.

@sffc sffc merged commit e04f442 into unicode-org:main Oct 27, 2023
96 of 97 checks passed
@sffc sffc deleted the macroregions-hardcode branch October 27, 2023 21:18
@@ -352,7 +352,53 @@ UBool U_CALLCONV cleanup() {
return true;
}

static const char16_t* MACROREGION_HARDCODE[] = {
Copy link
Contributor

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{

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants