Navigation Menu

Skip to content
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

ByteBuffer: improve and simplify get* implementations #896

Merged
merged 5 commits into from Mar 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 14 additions & 22 deletions Sources/NIO/ByteBuffer-aux.swift
Expand Up @@ -25,19 +25,16 @@ extension ByteBuffer {
/// - index: The starting index of the bytes of interest into the `ByteBuffer`.
/// - length: The number of bytes of interest.
/// - returns: A `[UInt8]` value containing the bytes of interest or `nil` if the bytes `ByteBuffer` are not readable.
public func getBytes(at index0: Int, length: Int) -> [UInt8]? {
precondition(index0 >= 0, "index must not be negative")
precondition(length >= 0, "length must not be negative")
let index = index0 - self.readerIndex
guard index >= 0 && index <= self.readableBytes - length else {
public func getBytes(at index: Int, length: Int) -> [UInt8]? {
guard let range = self.rangeWithinReadableBytes(index: index, length: length) else {
return nil
}

return self.withUnsafeReadableBytes { ptr in
// this is not technically correct because we shouldn't just bind
// the memory to `UInt8` but it's not a real issue either and we
// need to work around https://bugs.swift.org/browse/SR-9604
Array<UInt8>(UnsafeRawBufferPointer(rebasing: ptr[index..<(index+length)]).bindMemory(to: UInt8.self))
Array<UInt8>(UnsafeRawBufferPointer(rebasing: ptr[range]).bindMemory(to: UInt8.self))
}
}

Expand Down Expand Up @@ -133,15 +130,13 @@ extension ByteBuffer {
/// - length: The number of bytes making up the string.
/// - returns: A `String` value containing the UTF-8 decoded selected bytes from this `ByteBuffer` or `nil` if
/// the requested bytes are not readable.
public func getString(at index0: Int, length: Int) -> String? {
precondition(index0 >= 0, "index must not be negative")
precondition(length >= 0, "length must not be negative")
let index = index0 - self.readerIndex
public func getString(at index: Int, length: Int) -> String? {
guard let range = self.rangeWithinReadableBytes(index: index, length: length) else {
return nil
}
return self.withUnsafeReadableBytes { pointer in
guard index >= 0 && index <= pointer.count - length else {
return nil
}
return String(decoding: UnsafeRawBufferPointer(rebasing: pointer[index..<(index+length)]), as: Unicode.UTF8.self)
assert(range.lowerBound >= 0 && (range.upperBound - range.lowerBound) <= pointer.count)
return String(decoding: UnsafeRawBufferPointer(rebasing: pointer[range]), as: Unicode.UTF8.self)
}
}

Expand Down Expand Up @@ -196,15 +191,12 @@ extension ByteBuffer {
/// - length: The number of bytes.
/// - returns: A `DispatchData` value deserialized from this `ByteBuffer` or `nil` if the requested bytes
/// are not readable.
public func getDispatchData(at index0: Int, length: Int) -> DispatchData? {
precondition(index0 >= 0, "index must not be negative")
precondition(length >= 0, "length must not be negative")
let index = index0 - self.readerIndex
public func getDispatchData(at index: Int, length: Int) -> DispatchData? {
guard let range = self.rangeWithinReadableBytes(index: index, length: length) else {
return nil
}
return self.withUnsafeReadableBytes { pointer in
guard index >= 0 && index <= pointer.count - length else {
return nil
}
return DispatchData(bytes: UnsafeRawBufferPointer(rebasing: pointer[index..<(index+length)]))
return DispatchData(bytes: UnsafeRawBufferPointer(rebasing: pointer[range]))
}
}

Expand Down
15 changes: 12 additions & 3 deletions Sources/NIO/ByteBuffer-core.swift
Expand Up @@ -569,9 +569,7 @@ public struct ByteBuffer {
/// - returns: A `ByteBuffer` containing the selected bytes as readable bytes or `nil` if the selected bytes were
/// not readable in the initial `ByteBuffer`.
public func getSlice(at index: Int, length: Int) -> ByteBuffer? {
precondition(index >= 0, "index must not be negative")
precondition(length >= 0, "length must not be negative")
guard index >= self.readerIndex && index <= self.writerIndex - length else {
guard index >= 0 && length >= 0 && index >= self.readerIndex && index <= self.writerIndex - length else {
return nil
}
let index = _toIndex(index)
Expand Down Expand Up @@ -775,3 +773,14 @@ extension ByteBuffer: Equatable {
}
}
}

extension ByteBuffer {
@inlinable
func rangeWithinReadableBytes(index: Int, length: Int) -> Range<Int>? {
let indexFromReaderIndex = index - self.readerIndex
guard indexFromReaderIndex >= 0 && length >= 0 && indexFromReaderIndex <= self.readableBytes - length else {
return nil
}
return indexFromReaderIndex ..< (indexFromReaderIndex+length)
}
}
13 changes: 5 additions & 8 deletions Sources/NIO/ByteBuffer-int.swift
Expand Up @@ -47,17 +47,14 @@ extension ByteBuffer {
/// - returns: An integer value deserialized from this `ByteBuffer` or `nil` if the bytes of interest are not
/// readable.
@inlinable
public func getInteger<T: FixedWidthInteger>(at index0: Int, endianness: Endianness = Endianness.big, as: T.Type = T.self) -> T? {
precondition(index0 >= 0, "index must not be negative")
let index = index0 - self.readerIndex
public func getInteger<T: FixedWidthInteger>(at index: Int, endianness: Endianness = Endianness.big, as: T.Type = T.self) -> T? {
guard let range = self.rangeWithinReadableBytes(index: index, length: MemoryLayout<T>.size) else {
return nil
}
return self.withUnsafeReadableBytes { ptr in
guard index >= 0 && index <= ptr.count - MemoryLayout<T>.size else {
return nil
}
var value: T = 0
withUnsafeMutableBytes(of: &value) { valuePtr in
valuePtr.copyMemory(from: UnsafeRawBufferPointer(start: ptr.baseAddress!.advanced(by: index),
count: MemoryLayout<T>.size))
valuePtr.copyMemory(from: UnsafeRawBufferPointer(rebasing: ptr[range]))
}
return _toEndianness(value: value, endianness: endianness)
}
Expand Down
9 changes: 3 additions & 6 deletions Sources/NIO/ByteBuffer-views.swift
Expand Up @@ -80,14 +80,11 @@ extension ByteBuffer {
/// - index: The index the view should start at
/// - length: The length of the view (in bytes)
/// - returns A view into a portion of a `ByteBuffer` or `nil` if the requested bytes were not readable.
public func viewBytes(at index0: Int, length: Int) -> ByteBufferView? {
precondition(index0 >= 0, "index must not be negative")
precondition(length >= 0, "length must not be negative")
let index = index0 - self.readerIndex
guard index >= 0 && index <= self.readableBytes - length else {
public func viewBytes(at index: Int, length: Int) -> ByteBufferView? {
guard index >= self.readerIndex && index <= self.writerIndex - length else {
return nil
}

return ByteBufferView(buffer: self, range: index0 ..< index0+length)
return ByteBufferView(buffer: self, range: index ..< (index + length))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use the range-generation function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa ByteBufferView uses (and has to use as Collection) absolute ByteBuffer indices so here, we're not interested in range relative to readable bytes but range relative to the whole ByteBuffer

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, I see that.

This represents a difficulty with the requirement that ByteBufferViews only index readable bytes: specifically, what is readable can change after the ByteBufferView is created, without invalidating the ByteBufferView itself. I don't think this particularly matters much, as this will only happen if you're going outside the standard NIO usage patterns, but it's worth being aware of.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa no, ByteBufferView holds a (private) ByteBuffer which is a value, so the readable bytes can't change.

}
}
8 changes: 1 addition & 7 deletions Sources/NIOFoundationCompat/ByteBuffer-foundation.swift
Expand Up @@ -48,10 +48,6 @@ extension ByteBuffer {
/// - length: The number of bytes to be read from this `ByteBuffer`.
/// - returns: A `Data` value containing `length` bytes or `nil` if there aren't at least `length` bytes readable.
public mutating func readData(length: Int) -> Data? {
precondition(length >= 0, "length must not be negative")
guard self.readableBytes >= length else {
return nil
}
return self.getData(at: self.readerIndex, length: length).map {
self.moveReaderIndex(forwardBy: length)
return $0
Expand All @@ -66,10 +62,8 @@ extension ByteBuffer {
/// - length: The number of bytes of interest
/// - returns: A `Data` value containing the bytes of interest or `nil` if the selected bytes are not readable.
public func getData(at index0: Int, length: Int) -> Data? {
precondition(length >= 0, "length must not be negative")
precondition(index0 >= 0, "index must not be negative")
let index = index0 - self.readerIndex
guard index >= 0 && index <= self.readableBytes - length else {
guard index >= 0 && length >= 0 && index <= self.readableBytes - length else {
return nil
}
return self.withUnsafeReadableBytesWithStorageManagement { ptr, storageRef in
Expand Down
13 changes: 11 additions & 2 deletions Tests/NIOTests/ByteBufferTest.swift
Expand Up @@ -944,19 +944,28 @@ class ByteBufferTest: XCTestCase {
XCTAssertNil(body(Int.max - 1, 2), file: file, line: line)
XCTAssertNil(body(1, Int.max), file: file, line: line)
XCTAssertNil(body(2, Int.max - 1), file: file, line: line)
XCTAssertNil(body(Int.max, Int.max), file: file, line: line)
XCTAssertNil(body(Int.min, Int.min), file: file, line: line)
XCTAssertNil(body(Int.max, Int.min), file: file, line: line)
XCTAssertNil(body(Int.min, Int.max), file: file, line: line)
}

func testIndexOrLengthFunc<T>(_ body: (Int) -> T?, file: StaticString = #file, line: UInt = #line) {
XCTAssertNil(body(Int.max))
XCTAssertNil(body(Int.max - 1))
XCTAssertNil(body(Int.min))
}

testIndexOrLengthFunc({ x in buf.getInteger(at: x) as UInt16? })
testIndexOrLengthFunc({ buf.readSlice(length: $0) })
testIndexOrLengthFunc({ buf.readBytes(length: $0) })
testIndexOrLengthFunc({ buf.readData(length: $0) })
testIndexOrLengthFunc({ buf.readSlice(length: $0) })
testIndexOrLengthFunc({ buf.readString(length: $0) })
testIndexOrLengthFunc({ buf.readDispatchData(length: $0) })

testIndexOrLengthFunc({ buf.getInteger(at: $0, as: UInt8.self) })
testIndexOrLengthFunc({ buf.getInteger(at: $0, as: UInt16.self) })
testIndexOrLengthFunc({ buf.getInteger(at: $0, as: UInt32.self) })
testIndexOrLengthFunc({ buf.getInteger(at: $0, as: UInt64.self) })
testIndexAndLengthFunc(buf.getBytes)
testIndexAndLengthFunc(buf.getData)
testIndexAndLengthFunc(buf.getSlice)
Expand Down