optimize Zip to only use one Pull, and test for coverage#3
optimize Zip to only use one Pull, and test for coverage#3dr2chase wants to merge 3 commits intoDeedleFake:masterfrom
Conversation
DeedleFake
left a comment
DeedleFake
left a comment
There was a problem hiding this comment.
Thanks for the PR. I have a few comments, but overall very nice. That should be quite a bit more efficient. Kind of makes me wonder if some API could be added at some point to convert a pull iterator back into its push equivalent for the remainder of the items so that it could be used in the case that seq2 continues after seq1 finishes.
transform.go
Outdated
| // Zip returns a new Seq that yields the values of seq1 and seq2 | ||
| // simultaneously. | ||
| func Zip[T1, T2 any](seq1 Seq[T1], seq2 Seq[T2]) Seq[Zipped[T1, T2]] { | ||
| func Zip2Pull[T1, T2 any](seq1 Seq[T1], seq2 Seq[T2]) Seq[Zipped[T1, T2]] { |
There was a problem hiding this comment.
If the new one is completely identical to the old one in terms of functionality, there's no reason to keep the old one around, too. If they're different, the godoc comments should explain how. If you want to keep it for benchmarking purposes, move it into a _test.go file like oldZip().
oldzip_test.go
Outdated
| import "testing" | ||
|
|
||
| func BenchmarkOldZip(b *testing.B) { | ||
| b.ReportAllocs() |
There was a problem hiding this comment.
Is there any particular reason to use this instead of just passing -benchmem to go test?
transform.go
Outdated
| // Zip returns a new Seq that yields the values of seq1 and seq2 | ||
| // simultaneously. | ||
| func Zip[T1, T2 any](seq1 Seq[T1], seq2 Seq[T2]) Seq[Zipped[T1, T2]] { | ||
| return func(body func(Zipped[T1, T2]) bool) { |
There was a problem hiding this comment.
I'm not thrilled with body as the name of the yield function. Why not just yield like everywhere else?
2 participants
Pull is expensive, even with the coroutine patch. Only one is necessary.