Skip to content

[se-0405] rename String.init(validatingUTF8:) #68423

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 5 commits into from
Jan 22, 2024

Conversation

glessard
Copy link
Contributor

@glessard glessard commented Sep 11, 2023

API renaming from SE-0405, namely:

public init?(validatingUTF8 cString: UnsafePointer<CChar>)

renamed to

public init?(validatingCString nullTerminatedUTF8: UnsafePointer<CChar>)

We expected to do a symbol switch using @_silgen_name, but that cannot be done at the moment. (See #68433). The most compact alternative is this, where the renamed initializer uses the deprecated symbol -- sadly resulting in a deprecation warning when building the standard library.

Follow-up to #68419, rdar://114999766

@glessard glessard marked this pull request as draft September 11, 2023 09:41
@glessard
Copy link
Contributor Author

@swift-ci please smoke test

@glessard glessard marked this pull request as ready for review September 11, 2023 21:49
@glessard glessard requested a review from allevato September 11, 2023 21:49
@glessard glessard added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented labels Sep 12, 2023
@glessard
Copy link
Contributor Author

@swift-ci please smoke test

@benrimmington
Copy link
Contributor

The existing init(cString:) has overloads for UInt8 code units:

extension String {
  public init(cString nullTerminatedUTF8: UnsafePointer<CChar>)
  public init(cString nullTerminatedUTF8: UnsafePointer<UInt8>)
  public init(cString nullTerminatedUTF8: [CChar])
  public init(cString nullTerminatedUTF8: [UInt8])
}

@available(*, deprecated)
extension String {
  public init(cString nullTerminatedUTF8: inout CChar)
  public init(cString nullTerminatedUTF8: inout UInt8)
  public init(cString nullTerminatedUTF8: String)
}
  • Should init(validatingCString:) have the same overloads?

  • Should CChar be changed to CSignedChar or Int8?

@glessard
Copy link
Contributor Author

glessard commented Sep 13, 2023

The decision to always typealias CChar to Int8 is documented here: https://github.com/apple/swift/blob/main/docs/HowSwiftImportsCAPIs.md#fundamental-types
It is not well explained, but at least the source has authority.

I have been wondering about the init(cString:) initializer. The CChar version is a StringProtocol requirement, and SubString has that version. The UInt8 version was added to String, but it's not clear why not to SubString or, for that matter, StringProtocol. I suspect it may have been an oversight -- the String version is needed much more often.

This being said, in the spirit of SE-0324, it seems like we should have UInt8 and Int8 versions of C string interop functions in general.

@al45tair al45tair removed their request for review October 3, 2023 14:02
@glessard glessard requested a review from a team as a code owner January 3, 2024 22:38
public init?(validatingCString nullTerminatedUTF8: UnsafePointer<CChar>) {
// FIXME: https://github.com/apple/swift/issues/68433 (rdar://115296219)
self.init(validatingUTF8: nullTerminatedUTF8)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would seem preferable to point the ABI entry point to String.init?(validatingCString:), but renaming initializers isn't supported by @_silgen_name. This solution will cause a warning when compiling the stdlib in Swift 6.

@glessard
Copy link
Contributor Author

glessard commented Jan 3, 2024

@swift-ci please test

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Looks great! Would be nice to take this opportunity to give that initializer docs, too 👍🏻

@@ -162,39 +200,59 @@ extension String {

@inlinable
@_alwaysEmitIntoClient
public init?(validatingUTF8 cString: [CChar]) {
guard let length = cString.firstIndex(of: 0) else {
public init?(validatingCString nullTerminatedUTF8: [CChar]) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we more or less duplicate the docs for the other validatingCString initializer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought. These are stop-gap api that should be removed once pointer conversions can be restricted, but the time horizon is so long that it makes sense to document them until then. I should probably have done that when adding them last year.

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 will do the documentation update you suggest in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressing the documentation issues in #71224

@glessard glessard merged commit 50b83c8 into swiftlang:main Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants