Skip to content

Commit cbe2c0b

Browse files
gribozavrgribozavr
authored
Merge pull request #3896 from apple/stdlib-fix-ArraySlice.removeLast-bug
StdlibCollectionUnittest: unbreak testing multiple collections in one file, and bugs found by the corrected tests
2 parents 8f81b21 + a61d618 commit cbe2c0b

File tree

8 files changed

+81
-37
lines changed

8 files changed

+81
-37
lines changed

‎stdlib/private/StdlibCollectionUnittest/CheckCollectionType.swift.gyb‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -536,10 +536,11 @@ extension TestSuite {
536536

537537
var testNamePrefix = testNamePrefix
538538

539-
if checksAdded.contains(#function) {
539+
if !checksAdded.insert(
540+
"\(testNamePrefix).\(C.self).\(#function)"
541+
).inserted {
540542
return
541543
}
542-
checksAdded.insert(#function)
543544

544545
addSequenceTests(
545546
testNamePrefix,
@@ -1198,10 +1199,12 @@ extension TestSuite {
11981199
${testConstraints('BidirectionalCollection')} {
11991200

12001201
var testNamePrefix = testNamePrefix
1201-
if checksAdded.contains(#function) {
1202+
1203+
if !checksAdded.insert(
1204+
"\(testNamePrefix).\(C.self).\(#function)"
1205+
).inserted {
12021206
return
12031207
}
1204-
checksAdded.insert(#function)
12051208

12061209
addCollectionTests(${forwardTestArgs})
12071210

@@ -1515,10 +1518,11 @@ extension TestSuite {
15151518

15161519
var testNamePrefix = testNamePrefix
15171520

1518-
if checksAdded.contains(#function) {
1521+
if !checksAdded.insert(
1522+
"\(testNamePrefix).\(C.self).\(#function)"
1523+
).inserted {
15191524
return
15201525
}
1521-
checksAdded.insert(#function)
15221526

15231527
addBidirectionalCollectionTests(${forwardTestArgs})
15241528

@@ -1568,10 +1572,11 @@ extension TestSuite {
15681572
) where
15691573
${testConstraints(collectionForTraversal(Traversal))} {
15701574

1571-
if checksAdded.contains(#function) {
1575+
if !checksAdded.insert(
1576+
"\(testNamePrefix).\(C.self).\(#function)"
1577+
).inserted {
15721578
return
15731579
}
1574-
checksAdded.insert(#function)
15751580

15761581
func toCollection(_ r: CountableRange<Int>) -> C {
15771582
return makeCollection(r.map { wrapValue(OpaqueValue($0)) })

‎stdlib/private/StdlibCollectionUnittest/CheckMutableCollectionType.swift.gyb‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,10 +119,11 @@ extension TestSuite {
119119

120120
var testNamePrefix = testNamePrefix
121121

122-
if checksAdded.contains(#function) {
122+
if !checksAdded.insert(
123+
"\(testNamePrefix).\(C.self).\(#function)"
124+
).inserted {
123125
return
124126
}
125-
checksAdded.insert(#function)
126127

127128
addCollectionTests(
128129
testNamePrefix,
@@ -708,10 +709,11 @@ self.test("\(testNamePrefix).partition/InvalidOrderings") {
708709

709710
var testNamePrefix = testNamePrefix
710711

711-
if checksAdded.contains(#function) {
712+
if !checksAdded.insert(
713+
"\(testNamePrefix).\(C.self).\(#function)"
714+
).inserted {
712715
return
713716
}
714-
checksAdded.insert(#function)
715717

716718
addMutableCollectionTests(
717719
testNamePrefix,
@@ -861,10 +863,11 @@ self.test("\(testNamePrefix).partition/DispatchesThrough_withUnsafeMutableBuffer
861863

862864
var testNamePrefix = testNamePrefix
863865

864-
if checksAdded.contains(#function) {
866+
if !checksAdded.insert(
867+
"\(testNamePrefix).\(C.self).\(#function)"
868+
).inserted {
865869
return
866870
}
867-
checksAdded.insert(#function)
868871

869872
addMutableBidirectionalCollectionTests(
870873
testNamePrefix,

‎stdlib/private/StdlibCollectionUnittest/CheckRangeReplaceableCollectionType.swift‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -475,10 +475,11 @@ extension TestSuite {
475475

476476
var testNamePrefix = testNamePrefix
477477

478-
if checksAdded.contains(#function) {
478+
if !checksAdded.insert(
479+
"\(testNamePrefix).\(C.self).\(#function)"
480+
).inserted {
479481
return
480482
}
481-
checksAdded.insert(#function)
482483

483484
addCollectionTests(
484485
testNamePrefix,
@@ -1185,10 +1186,11 @@ self.test("\(testNamePrefix).OperatorPlus") {
11851186

11861187
var testNamePrefix = testNamePrefix
11871188

1188-
if checksAdded.contains(#function) {
1189+
if !checksAdded.insert(
1190+
"\(testNamePrefix).\(C.self).\(#function)"
1191+
).inserted {
11891192
return
11901193
}
1191-
checksAdded.insert(#function)
11921194

11931195
addRangeReplaceableCollectionTests(
11941196
testNamePrefix,
@@ -1313,10 +1315,11 @@ self.test("\(testNamePrefix).removeLast(n: Int)/whereIndexIsBidirectional/remove
13131315

13141316
var testNamePrefix = testNamePrefix
13151317

1316-
if checksAdded.contains(#function) {
1318+
if !checksAdded.insert(
1319+
"\(testNamePrefix).\(C.self).\(#function)"
1320+
).inserted {
13171321
return
13181322
}
1319-
checksAdded.insert(#function)
13201323

13211324
addRangeReplaceableBidirectionalCollectionTests(
13221325
testNamePrefix,

‎stdlib/private/StdlibCollectionUnittest/CheckRangeReplaceableSliceType.swift‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,11 @@ extension TestSuite {
4343
var testNamePrefix = testNamePrefix
4444

4545
// Don't run the same tests twice.
46-
if checksAdded.contains(#function) {
46+
if !checksAdded.insert(
47+
"\(testNamePrefix).\(C.self).\(#function)"
48+
).inserted {
4749
return
4850
}
49-
checksAdded.insert(#function)
5051

5152
addRangeReplaceableCollectionTests(
5253
testNamePrefix,
@@ -177,10 +178,11 @@ extension TestSuite {
177178
var testNamePrefix = testNamePrefix
178179

179180
// Don't run the same tests twice.
180-
if checksAdded.contains(#function) {
181+
if !checksAdded.insert(
182+
"\(testNamePrefix).\(C.self).\(#function)"
183+
).inserted {
181184
return
182185
}
183-
checksAdded.insert(#function)
184186

185187
addRangeReplaceableSliceTests(
186188
testNamePrefix,
@@ -324,10 +326,11 @@ extension TestSuite {
324326
var testNamePrefix = testNamePrefix
325327

326328
// Don't run the same tests twice.
327-
if checksAdded.contains(#function) {
329+
if !checksAdded.insert(
330+
"\(testNamePrefix).\(C.self).\(#function)"
331+
).inserted {
328332
return
329333
}
330-
checksAdded.insert(#function)
331334

332335
addRangeReplaceableBidirectionalSliceTests(
333336
testNamePrefix,

‎stdlib/private/StdlibCollectionUnittest/CheckSequenceType.swift‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,10 +1469,11 @@ extension TestSuite {
14691469

14701470
var testNamePrefix = testNamePrefix
14711471

1472-
if checksAdded.contains(#function) {
1472+
if !checksAdded.insert(
1473+
"\(testNamePrefix).\(S.self).\(#function)"
1474+
).inserted {
14731475
return
14741476
}
1475-
checksAdded.insert(#function)
14761477

14771478
func makeWrappedSequence(_ elements: [OpaqueValue<Int>]) -> S {
14781479
return makeSequence(elements.map(wrapValue))
@@ -1789,9 +1790,9 @@ self.test("\(testNamePrefix).first/semantics") {
17891790
test.expected == nil ? nil : wrapValueIntoEquatable(test.element),
17901791
found,
17911792
stackTrace: SourceLocStack().with(test.loc))
1792-
if test.expected != nil {
1793+
if let expectedIdentity = test.expected {
17931794
expectEqual(
1794-
test.expected, (found as? MinimalEquatableValue)?.identity,
1795+
test.expected, extractValueFromEquatable(found!).identity,
17951796
"find() should find only the first element matching its predicate")
17961797
}
17971798
}

‎stdlib/public/core/Arrays.swift.gyb‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,8 +1038,9 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol {
10381038
where S.Iterator.Element == Element {
10391039

10401040
self = ${Self}(
1041-
_buffer: _Buffer(s._copyToContiguousArray()._buffer,
1042-
shiftedToStartIndex: 0))
1041+
_buffer: _Buffer(
1042+
s._copyToContiguousArray()._buffer,
1043+
shiftedToStartIndex: 0))
10431044
}
10441045

10451046
/// Creates a new array containing the specified number of a single, repeated
@@ -1337,21 +1338,27 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol {
13371338
self._buffer.count = newCount
13381339
}
13391340

1341+
%if Self == 'ArraySlice':
13401342
/// Removes and returns the last element of the array.
13411343
///
13421344
/// The array must not be empty.
13431345
///
13441346
/// - Returns: The element that was removed.
1345-
@discardableResult
1346-
public mutating func removeLast() -> Element {
1347+
public mutating func _customRemoveLast() -> Element? {
13471348
_precondition(count > 0, "can't removeLast from an empty ${Self}")
1348-
let i = count
1349+
// FIXME(performance): if `self` is uniquely referenced, we should remove
1350+
// the element as shown below (this will deallocate the element and
1351+
// decrease memory use). If `self` is not uniquely refeneced, the code
1352+
// below will make a copy of the storage, which is wasteful. Instead, we
1353+
// should just shrink the view without allocating new storage.
1354+
let i = endIndex
13491355
// We don't check for overflow in `i - 1` because `i` is known to be
13501356
// positive.
13511357
let result = self[i &- 1]
13521358
self.replaceSubrange((i &- 1)..<i, with: EmptyCollection())
13531359
return result
13541360
}
1361+
%end
13551362

13561363
/// Inserts a new element at the specified position.
13571364
///
@@ -2132,6 +2139,16 @@ extension ContiguousArray {
21322139
}
21332140
}
21342141

2142+
extension ArraySlice {
2143+
public // @testable
2144+
init(_startIndex: Int) {
2145+
self.init(
2146+
_buffer: _Buffer(
2147+
ContiguousArray()._buffer,
2148+
shiftedToStartIndex: _startIndex))
2149+
}
2150+
}
2151+
21352152
%for (Self, a_Self) in arrayTypes:
21362153
extension ${Self} {
21372154
@available(*, unavailable, message: "Please use init(repeating:count:) instead")

‎stdlib/public/core/RangeReplaceableCollection.swift.gyb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -969,6 +969,7 @@ extension RangeReplaceableCollection where Self : BidirectionalCollection {
969969
/// - Returns: The last element of the collection.
970970
///
971971
/// - Complexity: O(1)
972+
@discardableResult
972973
public mutating func removeLast() -> Iterator.Element {
973974
_precondition(!isEmpty, "can't remove last element from an empty collection")
974975
if let result = _customRemoveLast() {

‎validation-test/stdlib/Arrays.swift.gyb‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -781,8 +781,15 @@ ArrayTestSuite.test("${array_type}/AssociatedTypes") {
781781
}
782782
% end
783783

784-
% for array_type in all_array_types:
785-
% collection_or_slice = 'Slice' if 'Slice' in array_type else 'Collection'
784+
func ArraySliceWithNonZeroStartIndex<T>(_ elements: [T]) -> ArraySlice<T> {
785+
var r = ArraySlice<T>(_startIndex: 1000)
786+
r.append(contentsOf: elements)
787+
expectEqual(1000, r.startIndex)
788+
return r
789+
}
790+
791+
% for array_type in all_array_types + ['ArraySliceWithNonZeroStartIndex']:
792+
% collection_or_slice = 'Slice' if 'Slice' in array_type else 'Collection'
786793

787794
do {
788795
// `Array`, `ArraySlice`, and `ContiguousArrayBuffer` have no expectation of
@@ -793,6 +800,7 @@ do {
793800

794801
// Test MutableCollectionType conformance with value type elements.
795802
ArrayTestSuite.addMutableRandomAccessCollectionTests(
803+
"${array_type}.",
796804
makeCollection: { (elements: [OpaqueValue<Int>]) in
797805
return ${array_type}(elements)
798806
},
@@ -815,6 +823,7 @@ do {
815823

816824
// Test MutableCollectionType conformance with reference type elements.
817825
ArrayTestSuite.addMutableRandomAccessCollectionTests(
826+
"${array_type}.",
818827
makeCollection: { (elements: [LifetimeTracked]) in
819828
return ${array_type}(elements)
820829
},
@@ -843,6 +852,7 @@ do {
843852

844853
// Test RangeReplaceableCollectionType conformance with value type elements.
845854
ArrayTestSuite.addRangeReplaceableRandomAccess${collection_or_slice}Tests(
855+
"${array_type}.",
846856
makeCollection: { (elements: [OpaqueValue<Int>]) in
847857
return ${array_type}(elements)
848858
},
@@ -858,6 +868,7 @@ do {
858868

859869
// Test RangeReplaceableCollectionType conformance with reference type elements.
860870
ArrayTestSuite.addRangeReplaceableRandomAccess${collection_or_slice}Tests(
871+
"${array_type}.",
861872
makeCollection: { (elements: [LifetimeTracked]) in
862873
return ${array_type}(elements)
863874
},

0 commit comments

Comments
 (0)