Skip to content

Conversation

@CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Mar 16, 2017

Explanation: Calling drop(while:) after prefix() loses the prefix because the internal drop(while:) override grabs the underlying base iterator from _PrefixSequence and wraps it in a _DropWhileSequence. Instead, fall back to Sequence's implementation.

Scope: Affects consumers of the standard library that attempt to chain Sequence transformations that factor through _PrefixSequence.

Reviewer: @airspeedswift

SR Issue: SR-4240

Risk: Very low risk; As this is an optimization, removing this causes a fallback to the correct implementation.

Testing: Regression test included in patch.

(cherry picked from commit 81968e2)

@CodaFi CodaFi added this to the Swift 3.1 milestone Mar 16, 2017
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 16, 2017

@swift-ci please test

@dabrahams
Copy link
Contributor

@swift-ci Please benchmark

@swift-ci
Copy link
Contributor

Build comment file:

Build failed before running benchmark.


@palimondo
Copy link
Contributor

That OS X Build failed due to missing benchmark base, if I understand the log correctly?

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 16, 2017

I don't imagine we have any benchmarks that would measure the impact of this change either - and even if we did they would be benchmarking an incorrect implementation.

@dabrahams
Copy link
Contributor

@CodaFi Sorry, but I don't understand what this is supposed to fix. The test, at least, already passes for me without having merged your change.

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 19, 2017

Strange. If I run this in the REPL that ships with beta 4 (Swift 3.1, swiftlang-802.0.42.1, clang-802.0.36), I get this:

let xs = sequence(first: 1, next: {$0 < 10 ? $0 + 1 : nil})

xs: UnfoldSequence<Int, (Int?, Bool)> = { ...

Array(xs.prefix(3).drop(while: {$0 < 7})).isEmpty

$R0: Bool = false

@dabrahams
Copy link
Contributor

@CodaFi Sorry, I was not reading carefully enough. I see this has already been merged to master.

@dabrahams
Copy link
Contributor

@swift-ci Please benchmark

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better test would be:

expectEqualSequence([], Array(xs.prefix(3).drop(while: {$0 < 7})))

because if the test failed, you'd get some useful information out of it.

Copy link
Contributor Author

@CodaFi CodaFi Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can prepare a patch for master that does this. Do you want this to also land with this PR in 3.1 or does the current PR work on its own?

@swift-ci
Copy link
Contributor

Build comment file:

Build failed before running benchmark.


CodaFi added 2 commits March 22, 2017 13:01
Calling drop(while: ) after prefix() on a pure Sequence loses the
prefix, because in the internal drop(while: ) override grabs the
underlying base iterator from _PrefixSequence and wraps it in a
_DropWhileSequence.
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 22, 2017

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 9303255f7d44a936b962b78681b227e303422426
Test requested by - @CodaFi

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 9303255f7d44a936b962b78681b227e303422426
Test requested by - @CodaFi

@palimondo
Copy link
Contributor

Will this land in 3.1 before final version of Xcode 8.3 ships?

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2017

@dabrahams @airspeedswift Did this just miss the boat?

@tkremenek tkremenek merged commit 6a4f736 into swiftlang:swift-3.1-branch Apr 7, 2017
@CodaFi CodaFi deleted the pick-a-nominee branch April 7, 2017 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants