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

Simplify zip take 2 #27511

Closed
wants to merge 6 commits into from
Closed

Conversation

haampie
Copy link
Contributor

@haampie haampie commented Jun 9, 2018

This pr:

  1. Removes Zip2 since Zip1 is a valid base case
  2. Fixes and adds a test for an isdone bug for the Zip iterator.
  3. Flattens the state like the values: iterate(zip(a, b, c)) returns ((val_a, val_b, val_c), (state_a, state_b, state_c)).
  4. Removes a call to isdone(z.b) in the iterate function, since that is handled by recursion already when calling iterate(z.b, ...).
  5. Adds a macro that @iamed2 suggested to "bail on nothing"; there's this three-liner pattern y = produce(); y === nothing && return nothing; a, b = y all over the place, which is a, b = @something produce() with the macro. Yet I suspect people wouldn't like this macro in base, at least not with this name. Suggestions?
  6. Adds a note to the docs about stateful iterators & how they are not advanced when another iterator finishes in the current iteration.

@haampie haampie force-pushed the simplify-zip-take-2 branch 3 times, most recently from 5ed5cb8 to b88cdc2 Compare June 9, 2018 17:47
@haampie
Copy link
Contributor Author

haampie commented Jun 9, 2018

Apparently the DRY-fix was breaking inference, even on the current master.

This PR with the last commit:

> @code_warntype iterate(zip(1:2,1:2,1:2,1:2), (1,1,1,1))
Body::Union{Nothing, Tuple{NTuple{4,Int64},NTuple{4,Int64}}}

Current master of Julia:

> @code_warntype iterate(zip(1:2,1:2,1:2,1:2), (1,(1,(1,1))))
Body::Union{Nothing, Tuple{Tuple{Int64,Any,Vararg{Any,N} where N},Tuple{Int64,Tuple{Any,Union{Tuple{Any,Any}, Base.LegacyIterationCompat{_1,_2,_3} where _3 where _2 where _1}}}}}

I don't yet see an easy way to test inference for Union{Nothing, ...} types, so a test is yet to be added.

@Keno
Copy link
Member

Keno commented Jun 9, 2018

I don't yet see an easy way to test inference for Union{Nothing, ...} types, so a test is yet to be added.

You can use Compiler.typesubtract on the inferred type.

@haampie
Copy link
Contributor Author

haampie commented Jun 24, 2018

I've added the commit of #27516 with an inference test of the new implementation of zip. Hopefully this can be merged soon :)

@KristofferC
Copy link
Sponsor Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

martinholters added a commit that referenced this pull request Sep 20, 2018
martinholters added a commit that referenced this pull request Oct 4, 2018
@haampie
Copy link
Contributor Author

haampie commented Oct 4, 2018

Superseded by #29238

@haampie haampie closed this Oct 4, 2018
@haampie haampie deleted the simplify-zip-take-2 branch October 4, 2018 13:58
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.

4 participants