-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Remove "illegal" UnsafePointer casts from the stdlib. #3824
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,9 +25,13 @@ public func _stdlib_mkstemps(_ template: inout String, _ suffixlen: CInt) -> CIn | |
| var utf8 = template.nulTerminatedUTF8 | ||
| let (fd, fileName) = utf8.withUnsafeMutableBufferPointer { | ||
| (utf8) -> (CInt, String) in | ||
| let fd = mkstemps(UnsafeMutablePointer(utf8.baseAddress!), suffixlen) | ||
| let fileName = String(cString: utf8.baseAddress!) | ||
| return (fd, fileName) | ||
| return utf8.baseAddress!.withMemoryRebound( | ||
| to: CChar.self, capacity: Int(suffixlen)) { | ||
|
|
||
| let fd = mkstemps($0, suffixlen) | ||
| let fileName = String(cString: $0) | ||
| return (fd, fileName) | ||
| } | ||
| } | ||
| template = fileName | ||
| return fd | ||
|
|
@@ -97,11 +101,13 @@ public func _stdlib_select( | |
| let readAddr = readfds.baseAddress | ||
| let writeAddr = writefds.baseAddress | ||
| let errorAddr = errorfds.baseAddress | ||
| let bindAsFdSet = { (p: UnsafeMutablePointer<UInt>?) in | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to deserve a real function. You can define it right here if you want, y’know. |
||
| UnsafeMutableRawPointer(p)?.assumingMemoryBound(to: fd_set.self) } | ||
| return select( | ||
| _stdlib_FD_SETSIZE, | ||
| UnsafeMutablePointer(readAddr), | ||
| UnsafeMutablePointer(writeAddr), | ||
| UnsafeMutablePointer(errorAddr), | ||
| bindAsFdSet(readAddr), | ||
| bindAsFdSet(writeAddr), | ||
| bindAsFdSet(errorAddr), | ||
| timeout) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,7 +113,7 @@ public class _stdlib_Barrier { | |
| var _pthreadBarrier: _stdlib_pthread_barrier_t | ||
|
|
||
| var _pthreadBarrierPtr: UnsafeMutablePointer<_stdlib_pthread_barrier_t> { | ||
| return UnsafeMutablePointer(_getUnsafePointerToStoredProperties(self)) | ||
| return _getUnsafePointerToStoredProperties(self).assumingMemoryBound(to: _stdlib_pthread_barrier_t.self) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 80 columns (please break before the dot).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. Must've thought I was in an overlay. but I think you want me to indent the second line which continues the statement. |
||
| } | ||
|
|
||
| public init(threadCount: Int) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,10 +114,12 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable { | |
| /// | ||
| /// - parameter buffer: The buffer of bytes to append. The size is calculated from `SourceType` and `buffer.count`. | ||
| public mutating func append<SourceType>(_ buffer : UnsafeBufferPointer<SourceType>) { | ||
| self.append(UnsafePointer(buffer.baseAddress!), count: buffer.count * sizeof(SourceType.self)) | ||
| buffer.baseAddress!.withMemoryRebound(to: UInt8.self, capacity: buffer.count * sizeof(SourceType.self)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dispatch uses tabs for indentation 😕, let's be consistent with the style choice made in this file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, believe me I'm trying to be consistent. But I guess that means actively inserting tab characters in places.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes :( |
||
| self.append($0, count: buffer.count * sizeof(SourceType.self)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sizeof was a bug. I'll fix it. |
||
| } | ||
| } | ||
|
|
||
| private func _copyBytesHelper(to pointer: UnsafeMutablePointer<UInt8>, from range: CountableRange<Index>) { | ||
| private func _copyBytesHelper(to pointer: UnsafeMutableRawPointer, from range: CountableRange<Index>) { | ||
| var copiedCount = 0 | ||
| __dispatch_data_apply(__wrapped) { (data: __DispatchData, offset: Int, ptr: UnsafeRawPointer, size: Int) in | ||
| let limit = Swift.min((range.endIndex - range.startIndex) - copiedCount, size) | ||
|
|
@@ -172,8 +174,7 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable { | |
|
|
||
| guard !copyRange.isEmpty else { return 0 } | ||
|
|
||
| let pointer : UnsafeMutablePointer<UInt8> = UnsafeMutablePointer<UInt8>(buffer.baseAddress!) | ||
| _copyBytesHelper(to: pointer, from: copyRange) | ||
| _copyBytesHelper(to: buffer.baseAddress!, from: copyRange) | ||
| return copyRange.count | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -387,12 +387,10 @@ extension String { | |
| outputArray in | ||
| // FIXME: completePath(...) is incorrectly annotated as requiring | ||
| // non-optional output parameters. rdar://problem/25494184 | ||
| let outputNonOptionalName = AutoreleasingUnsafeMutablePointer<NSString?>( | ||
| UnsafeMutablePointer<NSString>(outputName) | ||
| ) | ||
| let outputNonOptionalArray = AutoreleasingUnsafeMutablePointer<NSArray?>( | ||
| UnsafeMutablePointer<NSArray>(outputArray) | ||
| ) | ||
| let outputNonOptionalName = unsafeBitCast(outputName, | ||
| to: AutoreleasingUnsafeMutablePointer<NSString?>.self) | ||
| let outputNonOptionalArray = unsafeBitCast(outputArray, | ||
| to: AutoreleasingUnsafeMutablePointer<NSArray?>.self) | ||
| return self._ns.completePath( | ||
| into: outputNonOptionalName, | ||
| caseSensitive: caseSensitive, | ||
|
|
@@ -505,7 +503,7 @@ extension String { | |
| var stop_ = false | ||
| body(line: line, stop: &stop_) | ||
| if stop_ { | ||
| UnsafeMutablePointer<ObjCBool>(stop).pointee = true | ||
| stop.pointee = true | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -541,7 +539,7 @@ extension String { | |
| var stop_ = false | ||
| body($0, self._range($1), self._range($2), &stop_) | ||
| if stop_ { | ||
| UnsafeMutablePointer($3).pointee = true | ||
| ($3).pointee = true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra parens? |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -823,7 +821,7 @@ extension String { | |
| freeWhenDone flag: Bool | ||
| ) { | ||
| self = NSString( | ||
| charactersNoCopy: UnsafeMutablePointer(utf16CodeUnitsNoCopy), | ||
| charactersNoCopy: UnsafeMutablePointer(mutating: utf16CodeUnitsNoCopy), | ||
| length: count, | ||
| freeWhenDone: flag) as String | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,8 +169,10 @@ extension SCNGeometryElement { | |
| case .polygon: | ||
| fatalError("Expected constant number of indices per primitive") | ||
| } | ||
| // FIXME: Data should have an initializer for raw pointers. | ||
| let bytePtr = unsafeBitCast(UnsafeRawPointer(indices), to: UnsafePointer<UInt8>.self) | ||
| self.init( | ||
| data: Data(bytes: UnsafePointer<UInt8>(indices), count: indexCount * sizeof(IndexType.self)), | ||
| data: Data(bytes: bytePtr, count: indexCount * sizeof(IndexType.self)), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, we can't use |
||
| primitiveType: primitiveType, | ||
| primitiveCount: primitiveCount, | ||
| bytesPerIndex: sizeof(IndexType.self)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -362,7 +362,7 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */> | |
| /// Retrieve the value the pointer points to. | ||
| @_transparent get { | ||
| // We can do a strong load normally. | ||
| return UnsafeMutablePointer<Pointee>(self).pointee | ||
| return unsafeBitCast(self, to: UnsafeMutablePointer<Pointee>.self).pointee | ||
| } | ||
| /// Set the value the pointer points to, copying over the previous value. | ||
| /// | ||
|
|
@@ -409,6 +409,9 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */> | |
| /// This is inherently unsafe; UnsafeMutablePointer assumes the | ||
| /// referenced memory has +1 strong ownership semantics, whereas | ||
| /// AutoreleasingUnsafeMutablePointer implies +0 semantics. | ||
| /// | ||
| /// - Warning: Accessing `pointee` as a type that is unrelated to | ||
| /// the underlying memory's bound type is undefined. | ||
| @_transparent public | ||
| init<U>(_ from: UnsafeMutablePointer<U>) { | ||
| self._rawValue = from._rawValue | ||
|
|
@@ -421,6 +424,9 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */> | |
| /// This is inherently unsafe; UnsafeMutablePointer assumes the | ||
| /// referenced memory has +1 strong ownership semantics, whereas | ||
| /// AutoreleasingUnsafeMutablePointer implies +0 semantics. | ||
| /// | ||
| /// - Warning: Accessing `pointee` as a type that is unrelated to | ||
| /// the underlying memory's bound type is undefined. | ||
| @_transparent public | ||
| init?<U>(_ from: UnsafeMutablePointer<U>?) { | ||
| guard let unwrapped = from else { return nil } | ||
|
|
@@ -431,6 +437,9 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */> | |
| /// | ||
| /// This is inherently unsafe because UnsafePointers do not imply | ||
| /// mutability. | ||
| /// | ||
| /// - Warning: Accessing `pointee` as a type that is unrelated to | ||
| /// the underlying memory's bound type is undefined. | ||
| @_transparent | ||
| init<U>(_ from: UnsafePointer<U>) { | ||
| self._rawValue = from._rawValue | ||
|
|
@@ -442,13 +451,32 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */> | |
| /// | ||
| /// This is inherently unsafe because UnsafePointers do not imply | ||
| /// mutability. | ||
| /// | ||
| /// - Warning: Accessing `pointee` as a type that is unrelated to | ||
| /// the underlying memory's bound type is undefined. | ||
| @_transparent | ||
| init?<U>(_ from: UnsafePointer<U>?) { | ||
| guard let unwrapped = from else { return nil } | ||
| self.init(unwrapped) | ||
| } | ||
| } | ||
|
|
||
| extension UnsafeMutableRawPointer { | ||
| /// Convert from `AutoreleasingUnsafeMutablePointer`. | ||
| @_transparent | ||
| public init<T>(_ from: AutoreleasingUnsafeMutablePointer<T>) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not failable |
||
| _rawValue = from._rawValue | ||
| } | ||
| } | ||
|
|
||
| extension UnsafeRawPointer { | ||
| /// Convert from `AutoreleasingUnsafeMutablePointer`. | ||
| @_transparent | ||
| public init<T>(_ from: AutoreleasingUnsafeMutablePointer<T>) { | ||
| _rawValue = from._rawValue | ||
| } | ||
| } | ||
|
|
||
| extension AutoreleasingUnsafeMutablePointer : CustomDebugStringConvertible { | ||
| /// A textual representation of `self`, suitable for debugging. | ||
| public var debugDescription: String { | ||
|
|
||
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.
Would you mind putting the ") {" on a separate line? Then you won't need a blank line.
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.
Actually, you can use
String.nulTerminatedUTF8CString, and then you won't need to rebind.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.
#3816 was merged, and the property
nulTerminatedUTF8is gone. Please switch toutf8CStringand I think you won't need to rebind the memory then.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.
Already reworked in #3816. I'll go through and annotate other such instances here. (Sorry if I'm stepping on toes; ignore if this isn't helpful.)
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.
Yeah, that's why I wanted your PR merged first. Unfortunately I didn't have permission to do it so had to post this older patch.
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 rebind is gone now.
Incidentally, I much prefer your suggested style, but I can never keep track of what we're supposed to do. Is there a style guideline for stdlib code?
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.
Sorry, we don't have a written document.
This would be the most complex case of wrapping the declaration:
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.
Nice. Those are good simple rules. I think that would suffice in a document or a Readme.md