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 SI-6584, Stream#distinct uses too much memory. #1535

Merged
merged 2 commits into from Nov 1, 2012
Merged

Fix SI-6584, Stream#distinct uses too much memory. #1535

merged 2 commits into from Nov 1, 2012

Conversation

paulp
Copy link
Contributor

@paulp paulp commented Oct 30, 2012

Nesting recursive calls in Stream is always a dicey business.

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/788/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1498/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1498/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/788/

@jsuereth
Copy link
Member

Failures look legitimate.

@paulp
Copy link
Contributor Author

paulp commented Oct 30, 2012

And overconfidence had been working out so well for me...

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

PLS REBUILD ALL

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

You're right about the doc comments I'm sure, but undoing other peoples' copy-pasting is not on my list of things to do more of.

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

But I will attempt to make my original material conform.

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

Geez, is it expected that I have to re-scaladoc the entire library to view any change? Any attempt to scaladoc Stream in isolation explodes horribly.

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/801/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1511/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1511/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/801/

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

PLS REBUILD ALL

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/802/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1512/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/802/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1512/

@paulp
Copy link
Contributor Author

paulp commented Oct 31, 2012

Review by @retronym

@retronym
Copy link
Member

retronym commented Nov 1, 2012

Looks like it wasn't all copy/paste: a few hand crafted, Stream specific phrases and examples were added by Derek Wyatt way back in DocSpree days. Those are left on the cutting room floor after this patch. Not sure if you meant that. I'd suggest that you revert those parts from this PR and we can address them in a separate change.

Sorry to have led you down this windy path.

The code changes LGTM.

@paulp
Copy link
Contributor Author

paulp commented Nov 1, 2012

Maybe this is why they don't let me near documentation, but I consider redundant docs as valuable as redundant code. Does Stream#reverse really merit a Stream specific example for us to watch bitrot? It's reverse.

@retronym
Copy link
Member

retronym commented Nov 1, 2012

I tend to agree with you. But it's not my area of expertise, either. I guess that "will terminate for infinite sequences" isn't an accessible description for all. I'd say the right compromise is to put a few expository examples in the class documentation. @heathermiller might have an opinion here.

But until then, I would decouple the doc changes from the functional changes (other than the obviously good parts of separating implementation comments from doc comments).

Nesting recursive calls in Stream is always a dicey business.
"There's nothing we can do about dropRight," you say?
Oh but there is.
@paulp
Copy link
Contributor Author

paulp commented Nov 1, 2012

PLS REBUILD ALL

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/813/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1523/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/813/

@paulp
Copy link
Contributor Author

paulp commented Nov 1, 2012

Rogue aborter strikes again!

@paulp
Copy link
Contributor Author

paulp commented Nov 1, 2012

PLS REBUILD ALL

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/814/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1523/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/814/

paulp added a commit that referenced this pull request Nov 1, 2012
Fix SI-6584, Stream#distinct uses too much memory.
@paulp paulp merged commit 6f273cb into scala:master Nov 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants