-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[stdlib] [WIP] Eliminate version of Array.append(contentsOf:) that takes a Collection #5521
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 4 commits
04c7d94
dbf4bf9
3677d8e
acfc744
c857dab
3074318
6b9c3a5
38bb800
e6d2eb8
abc83ad
fdd02f6
0936c51
1889fde
23f25e1
ee5ed7b
b538baa
d1e0877
0c92978
d5046a6
f42fa36
3a61fba
047f7e0
dab14d3
951395c
b02568f
9d62225
6116001
3185571
e58648a
755c0a6
02738a5
11a46ff
8459212
71a9292
4cc3397
464fee9
db27ccf
b236244
5022c25
a26254b
88a06fb
7b82cf8
ce60a22
fee0727
8c4b871
130e5fb
d37e93d
faa7c74
2b62ed2
bd34ea5
ea5b665
bb0af66
7301b79
2b24689
83b6d3c
fac59ce
bde054a
25cb143
7f29a33
63498f8
832aa38
a202795
adb05f5
9cebf0a
ef49338
f7b1cbc
6dbcfc9
4fbd3e6
6705350
2706904
d2f490e
3ddf178
0d09619
f130bb1
cd4a1eb
06b6ba6
6f25956
5d18d27
3828784
6dd2dda
359a664
46eb397
e1822bc
891729f
9a389d5
a598ed6
3fae2be
b95a34a
b5e3524
fd5b46d
e9319fa
e265cc6
861f52c
e36b52c
51dc142
c20e012
3a4eb3f
adb862d
9bb914a
b294198
57ffa60
87dd722
bd53d2a
822bb35
0a179db
fb35153
5330485
d1b1361
7db9ac9
66e4597
796df2d
f56f403
00c1947
4cb5aef
5da7e10
053ca89
eb78182
ad01c1e
2517701
9f8ccd4
ab81426
54754c5
4daf56b
5f9fe6f
3b388df
fca7b66
65688bd
dfed050
a8da5b8
7258861
88ecaad
f801430
02c665f
2c7aae4
7e1bc3c
fdaace5
a27d554
8bfe205
48c529b
b258ebe
954f7d0
6bea953
54752d4
645d262
9f30532
9db06ee
4d34975
a72eba1
b5da219
9b03f9d
1682052
709e12d
f19cbef
8f96606
0107840
32ebe2d
e6ada30
c7ab11f
2932aac
d406c4d
469cccd
e455085
a230bde
cc2ae7b
b6d8bbb
2b05dd9
af934f2
2eeb8f5
e4abdfc
0a7e1d5
1fc7dbe
b8d60e9
3d7cac5
fd3101e
71df32f
43ce2e7
2555135
064fda5
16bb0cd
9e6089a
4b24cd4
6c9f99f
584f47d
18adb53
41dde88
e65d867
412559a
b47eff1
ab56a69
1aeca5b
a5ae8a5
146a537
12cbf5d
d9624dc
673b297
c80559d
150d484
7268654
c3bce78
b1c793e
92dd5f5
ca7046e
c8ced0d
1c48b31
dc1b6ae
3b25d59
f484576
299db43
db520f2
f91e043
71fea23
ba49dac
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 |
---|---|---|
|
@@ -1279,7 +1279,7 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol { | |
|
||
/// Adds the elements of a sequence to the end of the array. | ||
/// | ||
/// Use this method to append the elements of a sequence to the end of an | ||
/// Use this method to append the elements of a sequence to the end of this | ||
/// array. This example appends the elements of a `Range<Int>` instance | ||
/// to an array of integers. | ||
/// | ||
|
@@ -1293,39 +1293,8 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol { | |
/// - Complexity: O(*n*), where *n* is the length of the resulting array. | ||
public mutating func append<S : Sequence>(contentsOf newElements: S) | ||
where S.Iterator.Element == Element { | ||
let oldCount = self.count | ||
let capacity = self.capacity | ||
let newCount = oldCount + newElements.underestimatedCount | ||
|
||
if newCount > capacity { | ||
self.reserveCapacity( | ||
Swift.max(newCount, _growArrayCapacity(capacity))) | ||
} | ||
_buffer._arrayAppendSequence(newElements) | ||
} | ||
|
||
// An overload of `append(contentsOf:)` that uses the += that is optimized for | ||
// collections. | ||
// FIXME(ABI)#13 (Performance): remove this entrypoint. The overload for `Sequence` should be | ||
// made optimal for this case, too. | ||
/// Adds the elements of a collection to the end of the array. | ||
/// | ||
/// Use this method to append the elements of a collection to the end of this | ||
/// array. This example appends the elements of a `Range<Int>` instance | ||
/// to an array of integers. | ||
/// | ||
/// var numbers = [1, 2, 3, 4, 5] | ||
/// numbers.append(contentsOf: 10...15) | ||
/// print(numbers) | ||
/// // Prints "[1, 2, 3, 4, 5, 10, 11, 12, 13, 14, 15]" | ||
/// | ||
/// - Parameter newElements: The elements to append to the array. | ||
/// | ||
/// - Complexity: O(*n*), where *n* is the length of the resulting array. | ||
public mutating func append<C : Collection>(contentsOf newElements: C) | ||
where C.Iterator.Element == Element { | ||
|
||
let newElementsCount = numericCast(newElements.count) as Int | ||
let newElementsCount = newElements.underestimatedCount | ||
|
||
let oldCount = self.count | ||
let capacity = self.capacity | ||
|
@@ -1338,8 +1307,13 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol { | |
Swift.max(newCount, _growArrayCapacity(capacity)) | ||
: newCount) | ||
|
||
(self._buffer.firstElementAddress + oldCount).initialize(from: newElements) | ||
self._buffer.count = newCount | ||
let buf = UnsafeMutableBufferPointer(start: _buffer.firstElementAddress + oldCount, count: newElementsCount) | ||
_buffer.count = newCount | ||
if let remainder = buf.initialize(from: newElements) { | ||
// there were elements that didn't fit in the existing buffer, | ||
// append them in slow sequence-only mode | ||
_buffer._arrayAppendSequence(IteratorSequence(remainder)) | ||
} | ||
} | ||
|
||
%if Self == 'ArraySlice': | ||
|
@@ -1624,23 +1598,22 @@ extension ${Self} { | |
return try body(&inoutBufferPointer) | ||
} | ||
|
||
@discardableResult | ||
public func _copyContents( | ||
initializing ptr: UnsafeMutablePointer<Element> | ||
) -> UnsafeMutablePointer<Element> { | ||
if let s = self._baseAddressIfContiguous { | ||
let count = self.count | ||
ptr.initialize(from: s, count: count) | ||
initializing buf: UnsafeMutableBufferPointer<Element> | ||
) -> Iterator? { | ||
// FIXME: decide if this really is a fatalError | ||
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 would nil base address be an error? An empty buffer is supposed to be nil. 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. "An empty buffer is supposed to be nil." Is it? Surely an empty buffer is just a buffer where |
||
guard var p = buf.baseAddress | ||
else { fatalError("Attempt to copy contents into nil buffer pointer") } | ||
if let s = _baseAddressIfContiguous { | ||
p.initialize(from: s, count: self.count) | ||
_fixLifetime(self._owner) | ||
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 wish all _fixLifetime calls came with a comment. In this case, AFAICT the issue is that we need a _fixLifetime bracketing the _baseAddressIfContiguous getter and all uses of the pointer it returns. 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. Confident enough of this for me to add this as the comment above this on? 😀 |
||
return ptr + count | ||
} else { | ||
var p = ptr | ||
for x in self { | ||
p.initialize(to: x) | ||
p += 1 | ||
} | ||
return p | ||
} | ||
return nil | ||
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 is confusing. The Sequence._copyContents interface claims to support under-sized buffer arguments. But this implementation clearly doesn't support that. Is there a subtle assumption being made that Array always correctly reports it's count, thus we don't need to handle under-sized buffers? I don't think we can make any assumptions here because UnsafeMutableBufferPointer.initialize<S: Sequence>(from: S) is public. 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 is the call into the appended collection, copying itself into the buffer of the appendee. The appended knows it didn't lie about it's count. The appendee is required to have allocated enough storage for it to append itself (though this should be documented as a precondition of initialize(from:), and checked here, will do that). This is different to the appendee collection, which can make no assumptions about the generic collection it received. That said, based on benchmarks so far, this optimization might not be worth it. |
||
} | ||
} | ||
%end | ||
|
@@ -1766,54 +1739,6 @@ extension ${Self} { | |
} | ||
} | ||
} | ||
|
||
// FIXME(ABI)#16 : remove this entrypoint. The functionality should be provided by | ||
// a `+=` operator on `RangeReplaceableCollection`. | ||
/// Appends the elements of a sequence to ${a_Self}. | ||
/// | ||
/// Use this operator to append the elements of a sequence to the end of | ||
/// ${a_Self} with same `Element` type. This example appends | ||
/// the elements of a `Range<Int>` instance to an array of integers. | ||
/// | ||
/// var numbers = [1, 2, 3, 4, 5] | ||
/// numbers += 10...15 | ||
/// print(numbers) | ||
/// // Prints "[1, 2, 3, 4, 5, 10, 11, 12, 13, 14, 15]" | ||
/// | ||
/// - Parameters: | ||
/// - lhs: The array to append to. | ||
/// - rhs: A collection or finite sequence. | ||
/// | ||
/// - Complexity: O(*n*), where *n* is the length of the resulting array. | ||
public func += < | ||
S : Sequence | ||
>(lhs: inout ${Self}<S.Iterator.Element>, rhs: S) { | ||
lhs.append(contentsOf: rhs) | ||
} | ||
|
||
// FIXME(ABI)#17 : remove this entrypoint. The functionality should be provided by | ||
// a `+=` operator on `RangeReplaceableCollection`. | ||
/// Appends the elements of a collection to ${a_Self}. | ||
/// | ||
/// Use this operator to append the elements of a collection to the end of | ||
/// ${a_Self} with same `Element` type. This example appends | ||
/// the elements of a `Range<Int>` instance to an array of integers. | ||
/// | ||
/// var numbers = [1, 2, 3, 4, 5] | ||
/// numbers += 10...15 | ||
/// print(numbers) | ||
/// // Prints "[1, 2, 3, 4, 5, 10, 11, 12, 13, 14, 15]" | ||
/// | ||
/// - Parameters: | ||
/// - lhs: The array to append to. | ||
/// - rhs: A collection. | ||
/// | ||
/// - Complexity: O(*n*), where *n* is the length of the resulting array. | ||
public func += < | ||
C : Collection | ||
>(lhs: inout ${Self}<C.Iterator.Element>, rhs: C) { | ||
lhs.append(contentsOf: rhs) | ||
} | ||
% end | ||
|
||
//===--- generic helpers --------------------------------------------------===// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -477,9 +477,11 @@ internal func += <Element, C : Collection>( | |
let oldCount = lhs.count | ||
let newCount = oldCount + numericCast(rhs.count) | ||
|
||
let buf: UnsafeMutableBufferPointer<Element> | ||
|
||
if _fastPath(newCount <= lhs.capacity) { | ||
buf = UnsafeMutableBufferPointer(start: lhs.firstElementAddress + oldCount, count: numericCast(rhs.count)) | ||
lhs.count = newCount | ||
(lhs.firstElementAddress + oldCount).initialize(from: rhs) | ||
} | ||
else { | ||
var newLHS = _ContiguousArrayBuffer<Element>( | ||
|
@@ -490,7 +492,13 @@ internal func += <Element, C : Collection>( | |
from: lhs.firstElementAddress, count: oldCount) | ||
lhs.count = 0 | ||
swap(&lhs, &newLHS) | ||
(lhs.firstElementAddress + oldCount).initialize(from: rhs) | ||
buf = UnsafeMutableBufferPointer(start: lhs.firstElementAddress + oldCount, count: numericCast(rhs.count)) | ||
} | ||
|
||
if let remainder = buf.initialize(from: rhs) { | ||
// there were elements that didn't fit in the existing buffer, | ||
// append them in slow sequence-only mode | ||
//FIXME: handle this possibility | ||
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. The PR title should have [WIP] if you don't actually intend to merge this as-is. That said, why do you want to handle this case? Shouldn't we be consistent and make it a fatal error anywhere that Collection.count does not match the collections number of elements (assuming no intervening access to the Collection)? 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. Ah, you're right, this function is taking a collection so underflow should trap. |
||
} | ||
} | ||
|
||
|
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.
Doesn't this have the same correctness hole as append(contentsOf:)? The Data API can't guarantee that the generic collection argument is implemented correctly. In my mind we at least need an assert. Then it's a question of whether, in release mode, we want to crash with a fatalError or drop elements. Why not make it a fatalError?
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.
You're right, it needs to trap, especially for underflow in this case since it if it doesn't use the entire expected range it will leave corrupt data behind (specific to this being a replaceSubrange use case).