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

Fix Streaming#memoization. #665

Merged
merged 4 commits into from
Nov 19, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions core/src/main/scala/cats/data/Streaming.scala
Original file line number Diff line number Diff line change
Expand Up @@ -642,14 +642,21 @@ sealed abstract class Streaming[A] extends Product with Serializable { lhs =>
* Ensure that repeated traversals of the stream will not cause
* repeated tail computations.
*
* By default stream does not memoize to avoid memory leaks when the
* head of the stream is retained.
* By default this structure does not memoize to avoid memory leaks
* when the head of the stream is retained. However, the user
* ultimately has control of the memoization approach based on what
* kinds of Eval instances they use.
*
* There are two calls to memoized here -- one is a recursive call
* to this method (on the tail) and the other is a call to memoize
* the Eval instance holding the tail. For more information on how
* .memoize works see Eval#memoize.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: could we turn this into a proper ScalaDoc link? [[cats.Eval.memoize]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sure.

*/
def memoize: Streaming[A] =
this match {
case Empty() => Empty()
case Wait(lt) => Wait(lt.memoize)
case Cons(a, lt) => Cons(a, lt.memoize)
case Wait(lt) => Wait(lt.map(_.memoize).memoize)
case Cons(a, lt) => Cons(a, lt.map(_.memoize).memoize)
}

/**
Expand Down
18 changes: 18 additions & 0 deletions tests/src/test/scala/cats/tests/StreamingTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,24 @@ class StreamingTests extends CatsSuite {

class AdHocStreamingTests extends CatsSuite {

test("results aren't reevaluated after memoize") {
forAll { (orig: Streaming[Int]) =>
val ns = orig.toList
val size = ns.size

var i = 0
val memoized = orig.map { n => i += 1; n }.memoize

val xs = memoized.toList
i should === (size)
xs should === (ns)

val ys = memoized.toList
i should === (size)
ys should === (ns)
}
}

// convert List[A] to Streaming[A]
def convert[A](as: List[A]): Streaming[A] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere, or just added for possible future use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, looks like I left that in. I'll remove it.

Streaming.fromList(as)
Expand Down