Skip to content

Add support for UnsafeRawBufferPointer to DispatchData #230

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

Merged
merged 2 commits into from
Mar 10, 2017
Merged

Add support for UnsafeRawBufferPointer to DispatchData #230

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c80c9f
Add support for UnsafeRawBufferPointer to DispatchData
ktopley-apple Mar 9, 2017
5db517c
Review comments.
ktopley-apple Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/swift/Data.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,49 @@ public struct DispatchData : RandomAccessCollection {
/// Initialize a `Data` with copied memory content.
///
/// - parameter bytes: A pointer to the memory. It will be copied.
@available(swift, deprecated: 4, message: "Use init(bytes: UnsafeRawBufferPointer) instead")
public init(bytes buffer: UnsafeBufferPointer<UInt8>) {
let d = buffer.baseAddress == nil ? _swift_dispatch_data_empty()
: dispatch_data_create(buffer.baseAddress!, buffer.count, nil,
_dispatch_data_destructor_default())
self.init(data: d)
}

/// Initialize a `Data` with copied memory content.
///
/// - parameter bytes: A pointer to the memory. It will be copied.
/// - parameter count: The number of bytes to copy.
public init(bytes buffer: UnsafeRawBufferPointer) {
let d = buffer.baseAddress == nil ? _swift_dispatch_data_empty()
: dispatch_data_create(buffer.baseAddress!, buffer.count, nil,
_dispatch_data_destructor_default())
self.init(data: d)
}

/// Initialize a `Data` without copying the bytes.
///
/// - parameter bytes: A buffer pointer containing the data.
/// - parameter deallocator: Specifies the mechanism to free the indicated buffer.
@available(swift, deprecated: 4, message: "Use init(bytes: UnsafeRawBufferPointer, deallocater: Deallocator) instead")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to say "bytesNoCopy"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch.

public init(bytesNoCopy bytes: UnsafeBufferPointer<UInt8>, deallocator: Deallocator = .free) {
let (q, b) = deallocator._deallocator
let d = bytes.baseAddress == nil ? _swift_dispatch_data_empty()
: dispatch_data_create(bytes.baseAddress!, bytes.count, q?.__wrapped, b)
self.init(data: d)
}

/// Initialize a `Data` without copying the bytes.
///
/// - parameter bytes: A pointer to the bytes.
/// - parameter count: The size of the bytes.
/// - parameter deallocator: Specifies the mechanism to free the indicated buffer.
public init(bytesNoCopy bytes: UnsafeRawBufferPointer, deallocator: Deallocator = .free) {
let (q, b) = deallocator._deallocator
let d = bytes.baseAddress == nil ? _swift_dispatch_data_empty()
: dispatch_data_create(bytes.baseAddress!, bytes.count, q?.__wrapped, b)
self.init(data: d)
}

internal init(data: dispatch_data_t) {
__wrapped = __DispatchData(data: data, owned: true)
}
Expand Down Expand Up @@ -113,11 +138,23 @@ public struct DispatchData : RandomAccessCollection {
///
/// - parameter bytes: A pointer to the bytes to copy in to the data.
/// - parameter count: The number of bytes to copy.
@available(swift, deprecated: 4, message: "Use append(_: UnsafeRawBufferPointer) instead")
public mutating func append(_ bytes: UnsafePointer<UInt8>, count: Int) {
let data = dispatch_data_create(bytes, count, nil, _dispatch_data_destructor_default())
self.append(DispatchData(data: data))
}

/// Append bytes to the data.
///
/// - parameter bytes: A pointer to the bytes to copy in to the data.
/// - parameter count: The number of bytes to copy.
public mutating func append(_ bytes: UnsafeRawBufferPointer) {
// Nil base address does nothing.
guard bytes.baseAddress != nil else { return }
let data = dispatch_data_create(bytes.baseAddress!, bytes.count, nil, _dispatch_data_destructor_default())
self.append(DispatchData(data: data))
}

/// Append data to the data.
///
/// - parameter data: The data to append to this data.
Expand Down Expand Up @@ -156,19 +193,41 @@ public struct DispatchData : RandomAccessCollection {
/// - parameter pointer: A pointer to the buffer you wish to copy the bytes into.
/// - parameter count: The number of bytes to copy.
/// - warning: This method does not verify that the contents at pointer have enough space to hold `count` bytes.
@available(swift, deprecated: 4, message: "Use copyBytes(to: UnsafeMutableRawBufferPointer, count: Int) instead")
public func copyBytes(to pointer: UnsafeMutablePointer<UInt8>, count: Int) {
_copyBytesHelper(to: pointer, from: 0..<count)
}

/// Copy the contents of the data to a pointer.
///
/// - parameter pointer: A pointer to the buffer you wish to copy the bytes into.
/// - parameter count: The number of bytes to copy.
/// - warning: This method does not verify that the contents at pointer have enough space to hold `count` bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should do that, now that you're passing a buffer pointer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Stepping back, does it also make sense to default the count to the entire data, or would someone just not write that? How do I do an offset?

Stepping back even further…is this a necessary API at all? Normally in Swift we'd write this as

buffer.append(data[0..<count])

and if that's not going to be as efficient we should try to figure out what to do there. cc @airspeedswift

Copy link
Contributor Author

@ktopley-apple ktopley-apple Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it should do that, now that you're passing a buffer pointer?

Yes, I think it should probably copy min(buffer count, given count) [and that behavior will be documented].

Regarding the other comments, this change is intended to be the minimal change to allow us to support UnsafeRawBufferPointer. We want to keep the API in line with that of Foundation's Data and to that end I don't want to add anything new until Foundation has agreed to do the same. There will be more work in this area in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you know if it copied the whole thing? I'd prefer to assert, so that clients are encouraged to check the length beforehand.

Copy link
Member

Choose a reason for hiding this comment

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

This appears to duplicate the role of UnsafeMutableRawBufferPointer.initializeMemory<S: Sequence>(as: S.Iterator.Element.Type, from source: S). There shouldn't be a performance penalty if we do the _copyContents trick (which it looks like UnsafeMutableRawBufferPointer isn't taking advantage of, but should). We should probably revisit this. cc @phausler

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'd prefer to assert, so that clients are encouraged to check the length beforehand.

That seems like a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stepping back even further…is this a necessary API at all? Normally in Swift we'd write this as

buffer.append(data[0..<count])

This kind of thing is not likely to be efficient because the DispatchData could be composed of several discontiguous regions which would have to first be flattened (by copy) before being sliced and copied again (unless I am misunderstanding how that statement would work). The existing copy code handles the discontinuity without additional copies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Slicing doesn't immediately flatten, but append might go byte-by-byte by default. Ben would know more than me, though.

public func copyBytes(to pointer: UnsafeMutableRawBufferPointer, count: Int) {
guard pointer.baseAddress != nil else { return }
_copyBytesHelper(to: pointer.baseAddress!, from: 0..<count)
}

/// Copy a subset of the contents of the data to a pointer.
///
/// - parameter pointer: A pointer to the buffer you wish to copy the bytes into.
/// - parameter range: The range in the `Data` to copy.
/// - warning: This method does not verify that the contents at pointer have enough space to hold the required number of bytes.
@available(swift, deprecated: 4, message: "Use copyBytes(to: UnsafeMutableRawBufferPointer, from: CountableRange<Index>) instead")
public func copyBytes(to pointer: UnsafeMutablePointer<UInt8>, from range: CountableRange<Index>) {
_copyBytesHelper(to: pointer, from: range)
}

/// Copy a subset of the contents of the data to a pointer.
///
/// - parameter pointer: A pointer to the buffer you wish to copy the bytes into.
/// - parameter range: The range in the `Data` to copy.
/// - warning: This method does not verify that the contents at pointer have enough space to hold the required number of bytes.
public func copyBytes(to pointer: UnsafeMutableRawBufferPointer, from range: CountableRange<Index>) {
guard pointer.baseAddress != nil else { return }
_copyBytesHelper(to: pointer.baseAddress!, from: range)
}

/// Copy the contents of the data into a buffer.
///
/// This function copies the bytes in `range` from the data into the buffer. If the count of the `range` is greater than `MemoryLayout<DestinationType>.stride * buffer.count` then the first N bytes will be copied into the buffer.
Expand Down