Skip to content

[stdlib] non-mutating range subscript setter on UnsafeMutableBufferPointer #12504

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

Merged
merged 4 commits into from
Mar 6, 2018
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions stdlib/public/core/UnsafeBufferPointer.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,43 @@ public struct Unsafe${Mutable}BufferPointer<Element>
%end
}

/// Accesses a contiguous subrange of the buffer's elements.
///
/// The accessed slice uses the same indices for the same elements as the
/// original buffer uses. Always use the slice's `startIndex` property
/// instead of assuming that its indices start at a particular value.
///
/// This example demonstrates getting a slice from a buffer of strings, finding
/// the index of one of the strings in the slice, and then using that index
/// in the original buffer.
///
%if Mutable:
/// var streets = ["Adams", "Bryant", "Channing", "Douglas", "Evarts"]
/// streets.withUnsafeMutableBufferPointer { buffer in
/// let streetSlice = buffer[2..<buffer.endIndex]
/// print(Array(streetSlice))
/// // Prints "["Channing", "Douglas", "Evarts"]"
/// let index = streetSlice.index(of: "Evarts") // 4
/// buffer[index!] = "Eustace"
/// }
/// print(streets.last!)
/// // Prints "Eustace"
%else:
/// let streets = ["Adams", "Bryant", "Channing", "Douglas", "Evarts"]
/// streets.withUnsafeBufferPointer { buffer in
/// let streetSlice = buffer[2..<buffer.endIndex]
/// print(Array(streetSlice))
/// // Prints "["Channing", "Douglas", "Evarts"]"
/// let index = streetSlice.index(of: "Evarts") // 4
/// print(buffer[index!])
/// // Prints "Evarts"
/// }
%end
///
/// - Note: Bounds checks for `bounds` are performed only in debug mode.
///
/// - Parameter bounds: A range of the buffer's indices. The bounds of
/// the range must be valid indices of the buffer.
@_inlineable
public subscript(bounds: Range<Int>)
-> Slice<Unsafe${Mutable}BufferPointer<Element>>
Expand All @@ -260,11 +297,17 @@ public struct Unsafe${Mutable}BufferPointer<Element>
base: self, bounds: bounds)
}
% if Mutable:
set {
nonmutating set {
_debugPrecondition(bounds.lowerBound >= startIndex)
_debugPrecondition(bounds.upperBound <= endIndex)
_debugPrecondition(bounds.count == newValue.count)
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should be a full _precondition (similar to the one found in _writeBackMutableSlice).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The analog line in UnsafeRawBufferPointer uses a _debugPrecondition: https://github.com/apple/swift/blob/ffd34239a1b6f046346769a55ae8e717f64e13eb/stdlib/public/core/UnsafeRawBufferPointer.swift.gyb#L468
Is there a reason to make it stronger in the typed buffers, or should it be a _precondition in both places?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reasoning behind _debugPrecondition is that this is already an Unsafe type and being at the the lowest level of abstraction will be used in performance critical code.


// FIXME: swift-3-indexing-model: tests.
_writeBackMutableSlice(&self, bounds: bounds, slice: newValue)
if !newValue.isEmpty {
(_position! + bounds.lowerBound).assign(
from: newValue.base._position! + newValue.startIndex,
count: newValue.count)
}
}
% end
}
Expand Down
4 changes: 3 additions & 1 deletion stdlib/public/core/UnsafePointer.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,8 @@ public struct ${Self}<Pointee>: _Pointer {
/// `Pointee` must be a trivial type. After calling
/// `assign(from:count:)`, the region is initialized.
///
/// - Note: Returns without performing work if `self` and `source` are equal.
///
/// - Parameters:
/// - source: A pointer to at least `count` initialized instances of type
/// `Pointee`. The memory regions referenced by `source` and this
Expand All @@ -596,7 +598,7 @@ public struct ${Self}<Pointee>: _Pointer {
// self[i] = source[i]
// }
}
else {
else if UnsafePointer(self) != source {
// assign backward from a non-following overlapping range.
Builtin.assignCopyArrayBackToFront(
Pointee.self, self._rawValue, source._rawValue, count._builtinWordValue)
Expand Down
12 changes: 4 additions & 8 deletions validation-test/stdlib/CollectionType.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -580,21 +580,17 @@ CollectionTypeTests.test("subscript(_: Range<Index>)/writeback") {
CollectionTypeTests.test("subscript(_: Range<Index>)/defaultImplementation/sliceTooLarge")
.crashOutputMatches("Cannot replace a slice of a MutableCollection with a slice of a larger size")
.code {
var x = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
var x = Slice(base: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], bounds: 0..<10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed from MutableSlice to Slice. Conditional Conformances are nice.

expectCrashLater()
x.withUnsafeMutableBufferPointer { buffer in
buffer[2..<4] = buffer[4..<8]
}
x[2..<4] = x[4..<8]
}

CollectionTypeTests.test("subscript(_: Range<Index>)/defaultImplementation/sliceTooSmall")
.crashOutputMatches("Cannot replace a slice of a MutableCollection with a slice of a smaller size")
.code {
var x = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
var x = Slice(base: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], bounds: 0..<10)
expectCrashLater()
x.withUnsafeMutableBufferPointer { buffer in
buffer[2..<6] = buffer[6..<8]
}
x[2..<6] = x[6..<8]
}

//===----------------------------------------------------------------------===//
Expand Down
38 changes: 37 additions & 1 deletion validation-test/stdlib/UnsafeBufferPointer.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -757,8 +757,11 @@ UnsafeMutable${'Raw' if IsRaw else ''}BufferPointerTestSuite.test("subscript/${R
defer { deallocateFor${Raw}Buffer(
sliceMemory, count: replacementValues.count) }

// This produces a spurious compiler warning, someone take a look lol?
% if RangeName == 'range':
let buffer = SubscriptSetTest.create${SelfName}(from: memory)
% else:
var buffer = SubscriptSetTest.create${SelfName}(from: memory)
% end

% if IsRaw:
// A raw buffer pointer has debug bounds checks on indices, and
Expand Down Expand Up @@ -794,6 +797,39 @@ UnsafeMutable${'Raw' if IsRaw else ''}BufferPointerTestSuite.test("subscript/${R

% end # RangeName

UnsafeMutable${'Raw' if IsRaw else ''}BufferPointerTestSuite.test("subscript/set/overlaps") {
% if IsRaw:
let buffer = UnsafeMutableRawBufferPointer.allocate(count: 4)
% else:
let buffer = UnsafeMutableBufferPointer<Int>.allocate(capacity: 4)
% end
defer { buffer.deallocate() }

// Right Overlap
buffer[0] = 1
buffer[1] = 2
buffer[1..<3] = buffer[0..<2]
expectEqual(1, buffer[1])
expectEqual(2, buffer[2])
// Left Overlap
buffer[1] = 2
buffer[2] = 3
buffer[0..<2] = buffer[1..<3]
expectEqual(2, buffer[0])
expectEqual(3, buffer[1])
// Disjoint
buffer[2] = 2
buffer[3] = 3
buffer[0..<2] = buffer[2..<4]
expectEqual(2, buffer[0])
expectEqual(3, buffer[1])
buffer[0] = 0
buffer[1] = 1
buffer[2..<4] = buffer[0..<2]
expectEqual(0, buffer[2])
expectEqual(1, buffer[3])
}

% end # SelfName

runAllTests()