Skip to content
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

[SR-12648] RangeReplaceableCollection's 'filter' uses an intermediate array #55092

Closed
belkadan opened this issue Apr 21, 2020 · 8 comments
Closed
Labels
good first issue Good for newcomers improvement standard library Area: Standard library umbrella

Comments

@belkadan
Copy link
Contributor

Previous ID SR-12648
Radar None
Original Reporter @belkadan
Type Improvement
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Improvement, StarterBug
Assignee valeriyvan (JIRA)
Priority Medium

md5: d697e9673a4531a522d6c0386307389b

Issue Description:

RangeReplaceableCollection has an overload of filter that produces Self:

extension RangeReplaceableCollection {
  /// Returns a new collection of the same type containing, in order, the
  /// elements of the original collection that satisfy the given predicate.
  ///
  /// In this example, `filter(_:)` is used to include only names shorter than
  /// five characters.
  ///
  ///     let cast = ["Vivien", "Marlon", "Kim", "Karl"]
  ///     let shortNames = cast.filter { $0.count < 5 }
  ///     print(shortNames)
  ///     // Prints "["Kim", "Karl"]"
  ///
  /// - Parameter isIncluded: A closure that takes an element of the
  ///   sequence as its argument and returns a Boolean value indicating
  ///   whether the element should be included in the returned collection.
  /// - Returns: A collection of the elements that `isIncluded` allowed.
  ///
  /// - Complexity: O(*n*), where *n* is the length of the collection.
  @inlinable
  @available(swift, introduced: 4.0)
  public __consuming func filter(
    _ isIncluded: (Element) throws -> Bool
  ) rethrows -> Self {
    return try Self(self.lazy.filter(isIncluded))
  }
}

As written, this is intended to be constructing the new collection by lazily evaluating the predicate…but lazy filters don't support throwing closures, so this is actually just using the eager filter on Sequence that produces an Array. It should be replaced with a simple loop that appends elements one-by-one to a new Self value.

@swift-ci
Copy link
Collaborator

Comment by Pedro Carrasco (JIRA)

Hey! I'd like to try to tackle this issue. This would be my first time contributing to the Swift Language and I'll need some guidance on where to start, hints and/or anything that you believe I should know/be aware of.
Can anyone help me on this?

@belkadan
Copy link
Contributor Author

I'll try to help out. :-) The first step would be to get Swift building, as described in the Readme at https://github.com/apple/swift. You'd then make the change and write a test…but in this case I'm not actually sure how to write a test for this change. So my suggestion is to make the change and open a pull request, and ask at that point if the standard library reviewers have any ideas for testing it.

Oh, and you can assign this issue to yourself so someone else doesn't take it. Let me/us know if you have any specific questions!

@swift-ci
Copy link
Collaborator

Comment by Pedro Carrasco (JIRA)

Thanks @belkadan! I've already assigned this issue to me and I'll work on it on the upcoming days. I'll let you know if I have any problem �

@swift-ci
Copy link
Collaborator

swift-ci commented May 4, 2020

Comment by Pedro Carrasco (JIRA)

I haven't been able to find time to work on this but I'll try to open a PR this week. If I'm not able to do so, I'll remove myself from this issue so someone else can tackle it.

@swift-ci
Copy link
Collaborator

Comment by Pedro Carrasco (JIRA)

Turns out I'm still lacking time and I currently don't have enough disk storage to be able to properly build and test Swift.
I'll remove myself from this issue so I'm not blocking anyone. Thank you for everything @belkadan and I'm sorry! :/

@swift-ci
Copy link
Collaborator

Comment by Valeriy Van (JIRA)

PR #31911

@swift-ci
Copy link
Collaborator

Comment by Valeriy Van (JIRA)

merged

@swift-ci
Copy link
Collaborator

Comment by Valeriy Van (JIRA)

Fix was merged #31911

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers improvement standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

2 participants