Skip to content

Conversation

@szeiger
Copy link
Contributor

@szeiger szeiger commented Jul 5, 2016

== already covers this case. We override equals in Stream to do
the same when equals is called directly. This takes care of identical
streams.

To support short-circuiting equality checks on elements prepended to
identical streams we also override sameElements in Cons to treat
the case where both sides are Cons separately.

Tests in StreamTest.test_reference_equality.

Review by @Ichoran

JIRA: https://issues.scala-lang.org/browse/SI-6881

@scala-jenkins scala-jenkins added this to the 2.12.0-RC1 milestone Jul 5, 2016
`==` already covers this case. We override `equals` in `Stream` to do
the same when `equals` is called directly. This takes care of identical
streams.

To support short-circuiting equality checks on elements prepended to
identical streams we also override `sameElements` in `Cons` to treat
the case where both sides are `Cons` separately.

Tests in StreamTest.test_reference_equality.
@szeiger
Copy link
Contributor Author

szeiger commented Jul 7, 2016

Fixed stupid mistake...

@SethTisue
Copy link
Member

LGTM

I assume the use of a @tailrec helper is primarily about avoiding stack overflow, rather than primarily about saving a few CPU cycles here and there. you might add a comment to that effect before merging?

it took me a while to convince myself that a stack-safe sameElements actually needs to be this complicated, but I convinced myself. I think it could be improved sliiiiiightly by doing the eq check at the outer level of sameElements rather than inside the inner match. then in the case where I call sameElements directly (not from ==) and this and that are the same cons cell, you avoid an unnecessary comparison of the head values. but whatevs, it's a really tiny nitpick, feel free to change it or merge as-is, as you see fit

@szeiger
Copy link
Contributor Author

szeiger commented Aug 12, 2016

Doing the eq check at the outer level of sameElements is not sufficient because there is no recursion on sameElements anymore. Tails are checked with consEq instead, so we need to perform the eq check there.

@szeiger szeiger merged commit 14c02ac into scala:2.12.x Aug 12, 2016
@SethTisue
Copy link
Member

Doing the eq check at the outer level of sameElements is not sufficient

oops, I meant to say at the outer level of consEq. anyway, nbd

@szeiger
Copy link
Contributor Author

szeiger commented Aug 12, 2016

AFAIR I didnt't do that to avoid unnecessary comparisons when sameElements is called from equals or ==.

@adriaanm adriaanm added the 2.12 label Oct 29, 2016
julienrf added a commit to scala/collection-strawman that referenced this pull request Aug 16, 2017
Fix several issues with our implementation of LazyList.
cf scala/scala#4284 and scala/scala#5259
julienrf added a commit to scala/collection-strawman that referenced this pull request Aug 17, 2017
Fix several issues with our implementation of LazyList.
cf scala/scala#4284 and scala/scala#5259
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