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

Add remove(where:) to the Standard Library #675

Merged
merged 7 commits into from Jan 25, 2018

Conversation

Projects
None yet
8 participants
@airspeedswift
Member

airspeedswift commented Apr 8, 2017

No description provided.

@felix91gr

This comment has been minimized.

Show comment
Hide comment
@felix91gr

felix91gr Apr 26, 2017

Not in constant time, yes. But grapheme clusters have an upper bound of width, right? That would make the cost of the operation O(1), because even if itself is not constant, it's indeed bounded by one.
If I understood your point, you want guarantees of constant time. Those guarantees exist, if you look at it from a complexity analysis standpoint.
Therefore, I think I misunderstood why you propose this. This is an optimization after all, isn't it? That is a good reason. Also, this new syntaxis would be pretty nice, reducing errors from bad boilerplate code. I think that's what you meant? :)

Not in constant time, yes. But grapheme clusters have an upper bound of width, right? That would make the cost of the operation O(1), because even if itself is not constant, it's indeed bounded by one.
If I understood your point, you want guarantees of constant time. Those guarantees exist, if you look at it from a complexity analysis standpoint.
Therefore, I think I misunderstood why you propose this. This is an optimization after all, isn't it? That is a good reason. Also, this new syntaxis would be pretty nice, reducing errors from bad boilerplate code. I think that's what you meant? :)

This comment has been minimized.

Show comment
Hide comment
@airspeedswift

airspeedswift Apr 26, 2017

Owner

Nope, graphemes clusters can be of arbitrary length.

Owner

airspeedswift replied Apr 26, 2017

Nope, graphemes clusters can be of arbitrary length.

This comment has been minimized.

Show comment
Hide comment
@felix91gr

felix91gr Apr 26, 2017

@hartbit

This comment has been minimized.

Show comment
Hide comment
@hartbit

hartbit May 23, 2017

Collaborator

I dearly missed this yesterday. Any plans on getting it through review?

Collaborator

hartbit commented May 23, 2017

I dearly missed this yesterday. Any plans on getting it through review?

@Coeur

This comment has been minimized.

Show comment
Hide comment
@Coeur

Coeur Jun 21, 2017

Contributor

@airspeedswift

  • Preference would go for symmetry naming with filter: after all, it is not named filterAll. So a simple remove will do.

  • Introducing a mutating implementation for the removal job to be optimal means we could also introduce a mutating variant of filter with optimized results.

For the two reasons above, following conventions, I would go with:

// new in swift 4
public mutating func remove(_ isExcluded: (Element) throws -> Bool) rethrows
// new in swift 4
public mutating func filter(_ isIncluded: (Element) throws -> Bool) rethrows
// new in swift 4
public func removed(_ isExcluded: (Element) throws -> Bool) rethrows -> [Element]
// renamed from swift 3 `filter`
public func filtered(_ isIncluded: (Element) throws -> Bool) rethrows -> [Element]
Contributor

Coeur commented Jun 21, 2017

@airspeedswift

  • Preference would go for symmetry naming with filter: after all, it is not named filterAll. So a simple remove will do.

  • Introducing a mutating implementation for the removal job to be optimal means we could also introduce a mutating variant of filter with optimized results.

For the two reasons above, following conventions, I would go with:

// new in swift 4
public mutating func remove(_ isExcluded: (Element) throws -> Bool) rethrows
// new in swift 4
public mutating func filter(_ isIncluded: (Element) throws -> Bool) rethrows
// new in swift 4
public func removed(_ isExcluded: (Element) throws -> Bool) rethrows -> [Element]
// renamed from swift 3 `filter`
public func filtered(_ isIncluded: (Element) throws -> Bool) rethrows -> [Element]
@tkremenek

This comment has been minimized.

Show comment
Hide comment
@tkremenek

tkremenek Aug 9, 2017

Member

We discussed this in the Core Team meeting and this needs an implementation to be considered for review.

Member

tkremenek commented Aug 9, 2017

We discussed this in the Core Team meeting and this needs an implementation to be considered for review.

@dabrahams dabrahams changed the title from removeAll proposal to Add remove(where:) to the Standard Library Nov 15, 2017

* Proposal: [SE-NNNN](NNNN-filename.md)
* Authors: [Ben Cohen](https://github.com/airspeedswift)
* Review Manager: TBD
* Status: **Awaiting review**

This comment has been minimized.

@dabrahams

dabrahams Nov 15, 2017

Member

This should include a link to the implementation PR I think.

@dabrahams

dabrahams Nov 15, 2017

Member

This should include a link to the implementation PR I think.

@rjmccall rjmccall merged commit d737800 into apple:master Jan 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment