Sync NSStringAPI.swift with swift-corelibs-foundation #11474
Conversation
|
CC: @phausler |
|
this looks good to me, @moiseev @airspeedswift y'all may also want to chime in here |
| // | ||
| // If you change this file, you must update it in both places. | ||
|
|
||
| #if !DEPLOYMENT_RUNTIME_SWIFT |
There was a problem hiding this comment.
Can you please document this variable in the comment as well?
There was a problem hiding this comment.
This is the same define we use in Data and other Foundation overlay code that is shared. It is the demarcation that the underlying runtime is not objc; ala swift-corelibs-foundation.
It is worth noting that it is NOT the same as the #if _runtime(_ObjC) define since that is true when we build swift-corelibs-foundation on Mac; and we need a conditional to be a "this is being built for swift-corelibs-foundation"
| @@ -156,7 +170,7 @@ extension String { | |||
| /// C array of UTF8-encoded bytes. | |||
| public init?(utf8String bytes: UnsafePointer<CChar>) { | |||
| if let ns = NSString(utf8String: bytes) { | |||
| self = ns as String | |||
| self = String._unconditionallyBridgeFromObjectiveC(ns) | |||
There was a problem hiding this comment.
Is there any benefit to explicitly bridge here and elsewhere as opposed to implicitly bridge via as, other than... well, explicit-ness?
There was a problem hiding this comment.
as does not work on linux; but we do have _unconditionallyBridgeFromObjectiveC because we need to have the same functionality.
There was a problem hiding this comment.
Implicit bridging is not supported on Linux, so by explicitly bridging we can keep this code common and avoid more #ifs.
There was a problem hiding this comment.
_unconditionallyBridgeFromObjectiveC really seems like something that should not exist on Linux. Is there a reason you can't just use String(NSString)?
There was a problem hiding this comment.
we have to have a common protocol interface so that we can deal with the Any case. That basically boiled down to the concept that the _ObjectiveCBridgeable is public access level and was exposed API on these types. Converting to the initializers will be a penalty to the Darwin versions (which is a non-starter in my book).
There was a problem hiding this comment.
We need to continue the conversation about Linux bridging, but separately from this commit.
| @@ -227,7 +241,7 @@ extension String { | |||
| utf16CodeUnits: UnsafePointer<unichar>, | |||
| count: Int | |||
| ) { | |||
| self = NSString(characters: utf16CodeUnits, length: count) as String | |||
| self = String._unconditionallyBridgeFromObjectiveC(NSString(characters: utf16CodeUnits, length: count)) | |||
There was a problem hiding this comment.
Please keep it within 80 columns.
There was a problem hiding this comment.
That is the swift standard library formatting guidelines; Foundation is not limited as such (this file probably should be reformatted to match the rest of our style guides)
There was a problem hiding this comment.
Agreed with @phausler on this point, since we are the maintainers of this code.
|
@swift-ci Please test |
|
@ianpartridge, @phausler thanks for the explanations. So how is it going to be 'synced' between the repos? I guess it's not a symlink... May I suggest a test that diffs the two files and fails if they are different? |
|
I am not sure we can really enforce that; it just has to be hand rolled copy-pasta sadly... |
|
Could we one day merge the Foundation overlay with swift-corelibs-foundation? |
|
@slavapestov Almost certainly not - they are very different in many respects. The overlay assumes presence of ObjC, and the Darwin Foundation.framework. swift-corelibs-foundation is Foundation.framework itself. |
|
@parkera But if they share code, it might make sense to build both from the same source base/project, no? |
|
The Swift runtime on Darwin is still fairly deeply intertwined with the Foundation overlay for bridging, so I don't think we could realistically separate them. |
|
Any more thoughts on this PR, or can it be merged as-is? |
Over time,
NSStringAPI.swifthas diverged between the overlay and swift-corelibs-foundation.This PR updates the overlay so that the same file can be used in both projects. This should make future maintenance easier.