Skip to content

Enable string conversion in EUC-JP. #1296

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Enable string conversion in EUC-JP. #1296

wants to merge 6 commits into from
+358 −0

Conversation

YOCKOW
Copy link
Member

@YOCKOW YOCKOW commented May 16, 2025

Background: EUC-JP is not supported by OSS CoreFoundation, while it is supported by macOS Foundation Framework.
See #1016

This PR resolves the issue by calling ICU API if necessary.

@YOCKOW
Copy link
Member Author

YOCKOW commented May 16, 2025

@swift-ci Please test

@YOCKOW YOCKOW linked an issue May 16, 2025 that may be closed by this pull request
Discrepancy: String.data(using: .japaneseEUC) returns nil on non-Darwin. #1016
Open
@YOCKOW YOCKOW marked this pull request as ready for review May 16, 2025 09:25
@YOCKOW YOCKOW requested review from parkera and jmschonfeld May 16, 2025 09:26
parkera
parkera reviewed May 16, 2025
View reviewed changes
jmschonfeld
jmschonfeld reviewed May 16, 2025
View reviewed changes
@YOCKOW YOCKOW marked this pull request as draft May 20, 2025 03:09
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request May 20, 2025
@YOCKOW
ICU: Omit encodings that should be supported by FoundationEssentials.
b0a8981 
In response to: swiftlang#1296 (comment)
@YOCKOW YOCKOW force-pushed the i-see-you-legacy-string-encodings branch from 7ae3f7f to b0a8981 Compare May 20, 2025 03:45
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request May 20, 2025
@YOCKOW
ICU: Remove unnecessary nonisolated(unsafe) from static property.
5da8fc6 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request May 20, 2025
@YOCKOW
Add comment to func _icuMakeStringFromBytes_impl.
ba59831 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request May 20, 2025
@YOCKOW
Delegate string conversion to ICU only when encoding is EUC-JP.
96de131 
In response to: swiftlang#1296 (comment)
@YOCKOW
Copy link
Member Author

YOCKOW commented May 20, 2025

@swift-ci Please test

@YOCKOW
Copy link
Member Author

YOCKOW commented May 20, 2025

@parkera @jmschonfeld Thank you for reviewing.

In response to your feedback, I've made some changes:

  • Made it clear that ICU.StringConverter doesn't support UTF-* and US-ASCII.
  • Removed unnecessary nonisolated(unsafe) from the static property.
  • Added comments in func _icuMakeStringFromBytes_impl to clarify the safety of passing the pointer.
  • Let it delegate string conversion to ICU only if the encoding is EUC-JP and there's no FOUNDATION_FRAMEWORK (at this point).

@YOCKOW YOCKOW marked this pull request as ready for review May 20, 2025 08:16
@YOCKOW
Copy link
Member Author

YOCKOW commented May 27, 2025

Please let me request reviews again as per changes above.

@YOCKOW YOCKOW requested review from parkera and jmschonfeld May 27, 2025 02:19
parkera
parkera approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

I think this looks good to me, but also want @jmschonfeld to chime in.

jmschonfeld
jmschonfeld approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@jmschonfeld jmschonfeld left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for putting this together!

@jmschonfeld jmschonfeld self-requested a review June 11, 2025 20:07
jmschonfeld
jmschonfeld reviewed Jun 11, 2025
View reviewed changes
Sources/FoundationEssentials/String/String+IO.swift
// but are not supported by corelibs-foundation.
// We delegate conversion to ICU.
guard let string = (
bytes.withContiguousStorageIfAvailable({ _icuMakeStringFromBytes($0, encoding: encoding) }) ??
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually sorry one more thing that I just caught - this _icuMakeStringFromBytes function is only defined in !FOUNDATION_FRAMEWORK builds so this fails to build with FOUNDATION_FRAMEWORK enabled. Could you wrap this case in #if !FOUNDATION_FRAMEWORK. For now in Foundation.framework this will fall back to NSString below, and we can look at using ICU instead of NSString in Foundation.framework in the future (since that may be a behavioral change. Same goes for the case in StringProtocol+Essentials.swift as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the review. This is indeed wrapped by #if !FOUNDATION_FRAMEWORK already:

#if !FOUNDATION_FRAMEWORK
case .isoLatin1:
  ...
case .macOSRoman:
  ...
case .japaneseEUC:
  ...
#endif

Sources/FoundationInternationalization/ICU/ICU+StringConverter.swift
}


@_dynamicReplacement(for: _icuMakeStringFromBytes(_:encoding:))
Copy link
Contributor

Choose a reason for hiding this comment

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

This replaced function is only defined in !FOUNDATION_FRAMEWORK builds, could we wrap this in #if !FOUNDATION_FRAMEWORK as well so that it will build successfully in FOUNDATION_FRAMEWORK mode?

Copy link
Member Author

@YOCKOW YOCKOW Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is... not wrapped by #if. I'm going to fix that. Thank you for the catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request Jun 12, 2025
@YOCKOW
ICU: Omit encodings that should be supported by FoundationEssentials.
40c7c7d 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request Jun 12, 2025
@YOCKOW
ICU: Remove unnecessary nonisolated(unsafe) from static property.
c775018 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request Jun 12, 2025
@YOCKOW
Add comment to func _icuMakeStringFromBytes_impl.
65abdec 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request Jun 12, 2025
@YOCKOW
Delegate string conversion to ICU only when encoding is EUC-JP.
9733097 
In response to: swiftlang#1296 (comment)
YOCKOW added a commit to YOCKOW/swift-foundation that referenced this pull request Jun 12, 2025
@YOCKOW
Replace dynamic _icu* functions only if !FOUNDATION_FRAMEWORK.
bc1656d 
In response to: swiftlang#1296 (comment)
@YOCKOW YOCKOW force-pushed the i-see-you-legacy-string-encodings branch from 96de131 to bc1656d Compare June 12, 2025 07:41
YOCKOW added 6 commits June 12, 2025 16:47
@YOCKOW
Enable string conversion in EUC-JP.
5647484 
Background: EUC-JP is not supported by OSS CoreFoundation, while it is supported by macOS Foundation Framework.
See swiftlang#1016

This commit resolves the issue by calling ICU API if necessary.
@YOCKOW
ICU: Omit encodings that should be supported by FoundationEssentials.
c139e9a 
In response to: swiftlang#1296 (comment)
@YOCKOW
ICU: Remove unnecessary nonisolated(unsafe) from static property.
d9179ee 
In response to: swiftlang#1296 (comment)
@YOCKOW
Add comment to func _icuMakeStringFromBytes_impl.
b29ad2e 
In response to: swiftlang#1296 (comment)
@YOCKOW
Delegate string conversion to ICU only when encoding is EUC-JP.
be556f3 
In response to: swiftlang#1296 (comment)
@YOCKOW
Replace dynamic _icu* functions only if !FOUNDATION_FRAMEWORK.
844479e 
In response to: swiftlang#1296 (comment)
@YOCKOW YOCKOW force-pushed the i-see-you-legacy-string-encodings branch from bc1656d to 844479e Compare June 12, 2025 07:48
@YOCKOW
Copy link
Member Author

YOCKOW commented Jun 12, 2025

@swift-ci Please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmschonfeld jmschonfeld jmschonfeld approved these changes

@parkera parkera parkera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discrepancy: String.data(using: .japaneseEUC) returns nil on non-Darwin.
3 participants
@YOCKOW @parkera @jmschonfeld