-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add remove(where:) to the Standard Library #675
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
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1aac559
First draft of removeAll proposal
airspeedswift f4eefac
Remove Equatable version, switch to remove(where:)
airspeedswift 0f4a24d
eliminate a couple of missed All's
airspeedswift f1d36a9
Swift 4-ify
airspeedswift bffa4c5
tweak wording re implementation
airspeedswift 3e951c0
Linkify link to implementation
airspeedswift 44f7bf2
Fix my markdown
airspeedswift File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
# Adding in-place `remove(where:)` to the Standard Library | ||
|
||
* Proposal: [SE-NNNN](NNNN-filename.md) | ||
* Authors: [Ben Cohen](https://github.com/airspeedswift) | ||
* Review Manager: TBD | ||
* Status: **Awaiting review** | ||
* Implementation: [apple/swift#11576](https://github.com/apple/swift/pull/11576) | ||
|
||
## Introduction | ||
|
||
It is common to want to remove all occurrences of a certain element from a | ||
collection. This proposal is to add a `remove` algorithm to the | ||
standard library, which will remove all entries in a collection in-place | ||
matching a given predicate. | ||
|
||
## Motivation | ||
|
||
Removing all elements matching some criteria is a very common operation. | ||
However, it can be tricky to implement correctly and | ||
efficiently. | ||
|
||
The easiest way to achieve this effect in Swift 3 is to use `filter` and assign | ||
back, negating the thing you want to remove (because `filter` takes a closure | ||
of items to "keep"): | ||
|
||
```swift | ||
var nums = [1,2,3,4,5] | ||
// remove odd elements | ||
nums = nums.filter { !isOdd($0) } | ||
``` | ||
|
||
In addition to readability concerns, this has two performance problems: fresh | ||
memory allocation, and a copy of all the elements in full even if none need to | ||
be removed. | ||
|
||
The alternative is to open-code a `for` loop. The simplest performant solution | ||
is the "shuffle-down" approach. While not especially complex, it is certainly | ||
non-trivial: | ||
|
||
```swift | ||
if var i = nums.index(where: isOdd) { | ||
var j = i + 1 | ||
while j != nums.endIndex { | ||
let e = nums[j] | ||
if !isOdd(nums[j]) { | ||
nums[i] = nums[j] | ||
i += 1 | ||
} | ||
j += 1 | ||
} | ||
nums.removeSubrange(i...) | ||
} | ||
``` | ||
|
||
Possibilities for logic and performance errors abound. There are probably some | ||
in the above code. | ||
|
||
Additionally, this approach does not work for range-replaceable collections | ||
that are _not_ mutable i.e. collections that can replace subranges, but can't | ||
guarantee replacing a single element in constant time. `String` is the most | ||
important example of this, because its elements (graphemes) are variable width. | ||
|
||
## Proposed solution | ||
|
||
Add the following method to `RangeReplaceableCollection`: | ||
|
||
```swift | ||
nums.remove(where: isOdd) | ||
``` | ||
|
||
The default implementation will use the protocol's `init()` and `append(_:)` | ||
operations to implement a copy-based version. Collections which also conform to | ||
`MutableCollection` will get the more efficient "shuffle-down" implementation, | ||
but still require `RangeReplaceableCollection` as well because of the need to | ||
trim at the end. Other types may choose | ||
|
||
Collections which are range replaceable but _not_ mutable (like `String`) will | ||
be able to implement their own version which makes use of their internal | ||
layout. Collections like `Array` may also implement more efficient versions | ||
using memory copying operations. | ||
|
||
Since `Dictionary` and `Set` would benefit from this functionality as well, but | ||
are not range-replaceable, they should be given concrete implementations for | ||
consistency. | ||
|
||
## Detailed design | ||
|
||
Add the following to `RangeReplaceableCollection`: | ||
|
||
```swift | ||
protocol RangeReplaceableCollection { | ||
/// Removes every element satisfying the given predicate from the collection. | ||
mutating func remove(where: (Iterator.Element) throws -> Bool) rethrows | ||
} | ||
|
||
extension RangeReplaceableCollection { | ||
mutating func remove(where: (Iterator.Element) throws -> Bool) rethrows { | ||
// default implementation similar to self = self.filter | ||
} | ||
} | ||
``` | ||
|
||
Other protocols or types may also have custom implementations for a faster | ||
equivalent. For example `RangeReplaceableCollection where Self: | ||
MutableCollection` can provide a more efficient non-allocating default | ||
implementation. `String` is also likely to benefit from a custom implementation. | ||
|
||
## Source compatibility | ||
|
||
This change is purely additive so has no source compatibility consequences. | ||
|
||
## Effect on ABI stability | ||
|
||
This change is purely additive so has no ABI stability consequences. | ||
|
||
## Effect on API resilience | ||
|
||
This change is purely additive so has no API resilience consequences. | ||
|
||
## Alternatives considered | ||
|
||
`remove(where:)` takes a closure with `true` for elements to remove. | ||
`filter` takes a closure with elements to keep. In both cases, `true` is the | ||
"active" case, so likely to be what the user wants without having to apply a | ||
negation. The naming of `filter` is unfortunately ambiguous as to whether it's | ||
a removing or keeping operation, but re-considering that is outside the scope | ||
of this proposal. | ||
|
||
Several collection methods in the standard library (such as `index(where:)`) | ||
have an equivalent for collections of `Equatable` elements. A similar addition | ||
could be made that removes every element equal to a given value. This could | ||
easily be done as a further additive proposal later. | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This should include a link to the implementation PR I think.