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

[stdlib] SR-1519: Implement SE-0032: Add Sequence.first(predicate:) #2529

Merged
merged 5 commits into from May 16, 2016

Conversation

@russbishop
Copy link
Contributor

russbishop commented May 14, 2016

What's in this pull request?

Implementation of SE-0032, adds first() to Sequence protocol along with default implementation.

I modified FindTest to give each of the elements a separate identity so the find test can verify it found the first element and not just any matching element.

Resolved bug number: (SR-1519)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

@@ -950,13 +962,32 @@ extension Sequence {
///
/// - Parameter body: A closure that takes an element of the sequence as a
/// parameter.
@warn_unused_result

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

Looks like an unrelated edit... Even more, @warn_unused_result does nothing now.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

Sorry, my fault. I also didn't realize @discardable_result landed already.

@warn_unused_result
func first(
predicate: @noescape (Iterator.Element) throws -> Bool
) rethrows -> Iterator.Element?

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

For this part, we need a "dispatch" test in validation-test/stdlib/SequenceType.swift.gyb to ensure that we have a requirement.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

Ahhh I looked at this again and I think I see what the dispatch test is doing. I'll fix it.

self.sequence = sequence.map(MinimalEquatableValue.init)
var elementIndex = 0
self.sequence = sequence.map {
defer { elementIndex += 1 }

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

It is strongly discouraged to write side-effectful closures for map(). I'd recommend sequence.enumerated().map { ... }.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

No problem, I'll change it.

public func first(
predicate: @noescape (Iterator.Element) throws -> Bool
) rethrows -> Iterator.Element? {
for element in self {

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

Base it on forEach instead? forEach might be cheaper (by a constant factor) for some kinds of collections with complex iteration patterns, for example, trees.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

forEach can't break out of iterating the entire sequence which IMHO would be surprising behavior, especially for non-restartable or non-terminating sequences.

(I actually think the forEach API could use a termination flag for early exit but that's another discussion.)

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

forEach can't break out of iterating the entire sequence

We can throw from the closure passed to forEach :) I'm not sure if the semantic analysis will accept that and not confuse it with the rethrow semantics, but it is worth a shot.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

This didn't work at some point in the past but rethrow has gotten smarter since then. I tested that it doesn't interfere with throwing custom errors, nor does it break rethrows for a non-throwing closure.

I wanted to get your opinion on this before I push a commit since it looks "interesting" at first glance 😁

enum _ForEachStopIteration: ErrorProtocol {
    case stopIteration
}

extension Sequence {
    /// Calls the given closure on each element in the sequence in the same order
    /// as a `for`-`in` loop.
    ///
    /// - Parameter body: A closure that takes an element of the sequence as a
    ///   parameter. Return `true` from this closure to stop iteration.
    ///   Returning `false` will continue iteration.
    func _forEach(_ body: @noescape (Iterator.Element) throws -> Bool) rethrows {
        do {
            try self.forEach {
                if try body($0) {
                    throw _ForEachStopIteration.stopIteration
                }
            }
        } catch is _ForEachStopIteration { }
    }

    /// <insert find description here>
    public func find(where predicate: (Iterator.Element) throws -> Bool) rethrows -> Iterator.Element? {
        var foundElement: Iterator.Element? = nil
        try self._forEach {
            if try predicate($0) {
                foundElement = $0
                return true
            }
            return false
        }
        return foundElement
    }
}

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 15, 2016

Collaborator

This LGTM, except I'd probably prefer to inline _forEach into its only use site. return true and return false are less readable than just throwing and catching, I think.

This comment has been minimized.

Copy link
@russbishop

russbishop May 15, 2016

Author Contributor

That's fine with me. I was considering putting in a proposal for a public version of forEach that allows stopping but I don't care for returning a Bool from the closure and an inout parameter seemed strange so maybe it just doesn't fit as a stdlib API.

// first()
//===----------------------------------------------------------------------===//

SequenceTypeTests.test("first/Predicate") {

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

Please add this test to stdlib/private/StdlibCollectionUnittest/CheckSequenceType.swift instead, since first(where:) is a customization point and different sequences and collections can implement it differently.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

I'll move it. It still isn't quite clear to me exactly where the different kinds of tests belong. Is that stuff documented anywhere?

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

Sorry, it is not documented. The general principle is that we want to put all algorithm tests into the reusable StdlibCollectionUnittest module, so that we can maximize our algorithm and collection coverage.

/// - Returns: The first match or `nil` if there was no match.
@warn_unused_result
func first(
predicate: @noescape (Iterator.Element) throws -> Bool

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 14, 2016

Collaborator

The signature is first(where:) according to the proposal.

This comment has been minimized.

Copy link
@russbishop

russbishop May 14, 2016

Author Contributor

Also my fault for not realizing the ability to use keywords as parameters change already landed.

@russbishop
Copy link
Contributor Author

russbishop commented May 15, 2016

@gribozavr Pushed the other changes, all tests pass locally. Just waiting to get your opinion on the throw to stop iteration code.


SequenceTypeTests.test("first/dispatch") {
let tester = SequenceLog.dispatchTester([OpaqueValue(1)])
tester.first { return $0.value == 1 }

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 15, 2016

Collaborator

return is probably not needed here.

@russbishop
Copy link
Contributor Author

russbishop commented May 15, 2016

@gribozavr I think this is ready to go unless you can think of anything else

@@ -959,6 +970,34 @@ extension Sequence {
}
}

private enum _StopIteration: ErrorProtocol {

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 15, 2016

Collaborator

Sorry, one more thing -- we can't use private in the library due to a compiler bug. Please use internal.

Please add a space before the colon to conform to the library coding style.

@@ -959,6 +970,34 @@ extension Sequence {
}
}

private enum _StopIteration: ErrorProtocol {
case stop

This comment has been minimized.

Copy link
@gribozavr

gribozavr May 15, 2016

Collaborator

Indentation is two spaces.

This comment has been minimized.

Copy link
@russbishop

russbishop May 15, 2016

Author Contributor

Having separate tab guidelines between projects is killer :D

@gribozavr
Copy link
Collaborator

gribozavr commented May 15, 2016

@russbishop Thank you, LGTM modulo two style comments!

Russ Bishop
@russbishop
Copy link
Contributor Author

russbishop commented May 15, 2016

@gribozavr Done!

@gribozavr
Copy link
Collaborator

gribozavr commented May 15, 2016

@swift-ci Please test and merge

@gribozavr
Copy link
Collaborator

gribozavr commented May 15, 2016

@russbishop Could you take a look at the build errors?

Russ Bishop
@russbishop
Copy link
Contributor Author

russbishop commented May 15, 2016

@gribozavr Sorry about that, pushed a fix.

@gribozavr
Copy link
Collaborator

gribozavr commented May 15, 2016

@swift-ci Please test and merge

@gribozavr
Copy link
Collaborator

gribozavr commented May 16, 2016

CI failure is unrelated, merging.

@gribozavr gribozavr merged commit bebe750 into apple:master May 16, 2016
1 check failed
1 check failed
Test and Merge Build finished.
Details
@russbishop russbishop deleted the russbishop:se0032 branch May 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.