Skip to content

Upgrade to ICU 74 #39

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

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Conversation

iCharlesHu
Copy link
Contributor

This patches brings swift-foundation-icu to ICU 74, which is the same version shipped in macOS Sonoma

@iCharlesHu iCharlesHu force-pushed the charles/icu-74 branch 2 times, most recently from dd6e383 to 3969251 Compare July 29, 2024 19:54
@iCharlesHu
Copy link
Contributor Author

Toolchain build test: swiftlang/swift#75452

@@ -11,7 +11,7 @@
//

/*
These are defined before _foundation_unicode/uversion.h in order to prevent
These are defined before unicode/uversion.h in order to prevent

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not important here since it's just a comment, but should we tweak your script to apply another round of /s/unicode/_foundation_unicode after the merge, in case we miss places where this difference really matters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just testing unicode/*h might work... But I'm feeling somewhat nervous by that so I'd rather test #include unicode/*.h... IMO the comments are fine since we don't really manually edit the code anyways.

@@ -239,17 +239,17 @@ enum {

/* InsertPoints structure for noting where to put BiDi marks ---------------- */

typedef struct _Point {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this no longer an issue now?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed in private that this should no longer be an issue since #34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link

@itingliu itingliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed through headers and README and they look good

target_sources(_FoundationICU
PRIVATE
alphaindex.cpp
Copy link

@itingliu itingliu Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't seem to be new. Any idea why this wasn't included in this cmake file before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might have just forgotten to include it and got lucky with it 😓
Now these CMakeList files are auto generated so we shouldn't have this problem anymore.

@iCharlesHu
Copy link
Contributor Author

Squashed all commits

@iCharlesHu iCharlesHu merged commit b7cb010 into swiftlang:main Aug 1, 2024
@iCharlesHu iCharlesHu deleted the charles/icu-74 branch August 1, 2024 17:36
iCharlesHu added a commit to iCharlesHu/swift-foundation-icu that referenced this pull request Aug 1, 2024
iCharlesHu added a commit that referenced this pull request Aug 1, 2024
hyp added a commit to swiftlang/swift-installer-scripts that referenced this pull request Aug 2, 2024
hyp added a commit to swiftlang/swift-installer-scripts that referenced this pull request Aug 2, 2024
hyp added a commit to swiftlang/swift-installer-scripts that referenced this pull request Aug 5, 2024
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