ByteBuffer: improve and simplify get* implementations (#896)

Motivation:

For historic reasons, the get* implementations checked the same indices
repeatedly and the checks were also repetitive.

Modifications:

unify and de-deplicate get* range checks

Result:

fixes #884

Sources/NIO/ByteBuffer-aux.swift

@@ -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))
         }
     }
 
@@ -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)
         }
     }
 
@@ -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]))
         }
     }
 

Sources/NIO/ByteBuffer-core.swift

@@ -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)
@@ -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)
+    }
+}

Sources/NIO/ByteBuffer-int.swift

@@ -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)
         }

Sources/NIO/ByteBuffer-views.swift

@@ -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))
     }
 }

Sources/NIOFoundationCompat/ByteBuffer-foundation.swift

@@ -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
@@ -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

Tests/NIOTests/ByteBufferTest.swift

@@ -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)