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.

Sign up for GitHub

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

Jump to bottom

Change LazyList to always evaluate elements in order, and to have lazy empty-or-not status #7558

Merged
merged 1 commit into from
Jan 27, 2019
Merged

Change LazyList to always evaluate elements in order, and to have lazy empty-or-not status #7558

merged 1 commit into from
Jan 27, 2019

Conversation

NthPortal
Copy link
Contributor

Resolves scala/bug#11307.

@NthPortal NthPortal added WIP library:collections PRs involving changes to the standard collection library labels Dec 21, 2018
@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Dec 21, 2018
@NthPortal
Copy link
Contributor Author

I will try to get this sometime later this week

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 8, 2019
@Ichoran Ichoran self-assigned this Jan 10, 2019
@NthPortal NthPortal removed the WIP label Jan 11, 2019
@NthPortal NthPortal force-pushed the topic/ll-strict-head/PR branch 3 times, most recently from e496cd1 to 51cccd9 Compare January 11, 2019 04:10
@NthPortal
Copy link
Contributor Author

I think this is ready for review.

Left a few overrides calling super with TODOs to revisit for potential minor laziness improvements. Because the overrides are there, they don't need to be addressed before 2.13.0.

@SethTisue
Copy link
Member

(I don't see Scaladoc changes, shouldn't there be some of those?)

@NthPortal
Copy link
Contributor Author

NthPortal commented Jan 11, 2019 •
edited
Loading

@SethTisue the docs were already hopelessly out of date - this PR doesn't make them any more so. Updating them is a task for scala/bug#11131.

(translation: yes, there should be, but there aren't 😬)

@NthPortal NthPortal mentioned this pull request Jan 11, 2019
Closed
1 task
Ichoran
Ichoran suggested changes Jan 12, 2019
View reviewed changes
Copy link
Contributor

@Ichoran Ichoran left a comment

Choose a reason for hiding this comment

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

Overridden methods that don't need to be overridden should be removed (and in some cases the superclass implementation could be improved!), but otherwise this looks like a great simplification.

src/library/scala/collection/immutable/LazyList.scala Outdated
if (res < 0) res else reversed.length - 1 - res
}
// TODO: maybe re-implement to be lazy with `that` if head of `that` never matches elements of `this`
override def endsWith[B >: A](that: collection.Iterable[B]): Boolean = super.endsWith(that)
Copy link
Contributor

Choose a reason for hiding this comment

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

That could be arbitrarily expensive with all those comparisons that you otherwise could skip. I think the super implementation is the right one, and you should just remove this override and let it use the one from Seq. LinearSeq as a whole could probably use an override that sped up some common scenarios, but this is the PR for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you should fix the Seq one to early-return false if length is less than that.length. Lots of expensive stuff that could be skipped there. Also, to return true if that is empty (without evaluating the current collection at all, not even calling length). This is better for everyone, but especially important for avoiding unnecessarily eager behavior in LazyList.

src/library/scala/collection/immutable/LazyList.scala Outdated
if (res < 0) res else reversed.length - 1 - res
}
// TODO: maybe re-implement to be lazy with `that` if head of `that` never matches elements of `this`
override def lastIndexOfSlice[B >: A](that: collection.Seq[B], end: Int): Int =
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this method and let the superclass handle it. The Seq implementation already special-cases IndexedSeq and everything else; any benefits should be applied there. (It uses KMP searching, so it's nontrivial to improve on it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ichoran after more thought, I'm thinking that this method (and endsWith) should not attempt to preserve any laziness of that - would you agree?

@NthPortal
Copy link
Contributor Author

@Ichoran I believe I have addressed your comments

This was referenced Jan 17, 2019
Merged
Open
Ichoran
Ichoran approved these changes Jan 23, 2019
View reviewed changes
Copy link
Contributor

@Ichoran Ichoran left a comment

Choose a reason for hiding this comment

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

Changes LGTM--let's get this in and more thoroughly tested (e.g. with collections-laws)

@Ichoran
Copy link
Contributor

Ichoran commented Jan 23, 2019

Handled the merge conflict. Let's see if the tests go through.

@NthPortal
Copy link
Contributor Author

@Ichoran there's a test in LazyListTest that checks that updated throws eagerly; I'll remove it tonight since Stefan's PR already added a test for the correct behaviour.

@NthPortal
Copy link
Contributor Author

ping

@Ichoran Ichoran merged commit 5631a7b into scala:2.13.x Jan 27, 2019
@NthPortal NthPortal deleted the topic/ll-strict-head/PR branch January 27, 2019 18:59
@SethTisue
Copy link
Member

nice to have this settled 🙂

@SethTisue SethTisue changed the title Change LazyList to have a strict head Change LazyList to have a strict head (but lazy empty-or-not status) Feb 27, 2019
@SethTisue SethTisue changed the title Change LazyList to have a strict head (but lazy empty-or-not status) Change LazyList to 1) always evaluate elements in order 2) have lazy empty-or-not status Apr 4, 2019
@SethTisue SethTisue changed the title Change LazyList to 1) always evaluate elements in order 2) have lazy empty-or-not status Change LazyList to always evaluate elements in order and have lazy empty-or-not status Apr 4, 2019
@SethTisue SethTisue changed the title Change LazyList to always evaluate elements in order and have lazy empty-or-not status Change LazyList to always evaluate elements in order, and to have lazy empty-or-not status Apr 4, 2019
@SethTisue SethTisue changed the title Change LazyList to always evaluate elements in order, and to have lazy empty-or-not status Change LazyList to always evaluate elements in order, and to have lazy empty-or-not status Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ichoran Ichoran Ichoran approved these changes

Assignees

@Ichoran Ichoran

Labels
library:collections PRs involving changes to the standard collection library release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.0-RC1
4 participants
@NthPortal @SethTisue @Ichoran @scala-jenkins