-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[SE-0134][stdlib] Rename/remove two properties on String #3816
Conversation
// Subtract 1 so we don't get the null terminator byte. This matches NSString behavior. | ||
return Data(bytes: ptr.baseAddress!, count: ptr.count - 1) | ||
return string.utf8CString.withUnsafeBufferPointer { (ptr) in | ||
ptr.baseAddress!.withMemoryRebound(to: UInt8.self, capacity: ptr.count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, Data(bytes: UnsafeRawPointer, count: Int)
isn't available in this repo as it now is in swift-corelibs-foundation. Should I address that in this PR too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that issue is separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parkera What is the meaning of this? How can corelibs-foundation and Foundation overlay vend different APIs? I would love to change the Foundation overlay to "bytes: UnsafeRawPointer" so that it's usable without breaking the memory model. That change would be mostly source compatible, but, strictly speaking it is source breaking, thus verboten.
@lattner @atrick Would you mind reviewing? @tkremenek As requested on swift-evolution list, I'm pinging you because this is a Swift 3-related proposal implementation. |
mangledNameLength: UInt, | ||
outputBuffer: UnsafeMutablePointer<UInt8>?, | ||
outputBuffer: UnsafeMutablePointer<CChar>?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of my depth with @_silgen_name
; is this OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, your version of the code is more correct than what was there before...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is good.
Otherwise looks great. |
Could you add an unavailable declaration with a fixit to the new API? LGTM with that. Edit: never mind, found it. |
@swift-ci Please test |
OS X failure is unrelated. Linux failure is expected and is solved by simultaneous merge of swiftlang/swift-corelibs-foundation#486. |
@tkremenek Someone please merge this. I merged the corelibs change but I'm not authorized to merge this, so things will be broken until this merge happens. |
@gribozavr Please merge |
@swift-ci Please smoke test |
What's in this pull request?
This PR implements SE-0134. Merge simultaneously with swiftlang/swift-corelibs-foundation#486.
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.