Skip to content

Make MapView#values preserve laziness #9090

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
Merged
SethTisue merged 1 commit into from
Jul 2, 2020
Merged

Make MapView#values preserve laziness #9090

SethTisue merged 1 commit into from
Jul 2, 2020

Conversation

@NthPortal
Copy link
Contributor

@NthPortal NthPortal commented Jun 27, 2020

ChangeMapView#values to return a View instead
of an AbstractIterable, so that further operations on
the returned value remain lazy.

Fixes scala/bug#12059

@NthPortal NthPortal requested a review from julienrf June 27, 2020 22:04
@scala-jenkins scala-jenkins added this to the 2.13.4 milestone Jun 27, 2020
@NthPortal
Copy link
Contributor Author

NthPortal commented Jun 28, 2020 •
edited
Loading

community build at https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3551/ (outdated) eventually, unless I screwed up the job in which case not

@NthPortal NthPortal added the library:collections PRs involving changes to the standard collection library label Jun 28, 2020
julienrf
julienrf reviewed Jun 28, 2020
View reviewed changes
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @NthPortal!

Do we have something to change in SortedMap as well?

@NthPortal
Copy link
Contributor Author

NthPortal commented Jun 28, 2020

Do we have something to change in SortedMap as well?

no, it seems that SortedMap#view uses the same implementation. however, we may need to add something for keys

@NthPortal
[bug#12059] Make MapView#values and #keys return Views
e15f471 
Change`MapView#values` and `MapView#keys` to return `View`s
instead of `AbstractIterable`s, so that further operations on
the returned values remain lazy.
@NthPortal
Copy link
Contributor Author

NthPortal commented Jun 28, 2020

honestly, it would be nice if keySet could be lazy too, but it returns a Set, so that's not really possible

dwijnand
dwijnand reviewed Jun 29, 2020
View reviewed changes
build.sbt

// Fix for scala/bug#12059
ProblemFilters.exclude[MissingClassProblem]("scala.collection.MapView$Keys"),
ProblemFilters.exclude[MissingClassProblem]("scala.collection.MapView$Values"),
Copy link
Member

@dwijnand dwijnand Jun 29, 2020

Choose a reason for hiding this comment

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

Wouldn't it be better to avoid making these jvm-public classes by creating anonymous inner classes?

Copy link
Contributor Author

@NthPortal NthPortal Jul 1, 2020

Choose a reason for hiding this comment

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

I don't know. I see your point, but at the same time, I think it's cleaner the way it is. I'd love to know what others think cc @julienrf

Copy link
Contributor

@julienrf julienrf Jul 1, 2020

Choose a reason for hiding this comment

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

I have a slight preference for having (private) named classes

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jul 1, 2020
dwijnand
dwijnand previously requested changes Jul 1, 2020
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

If you'll squash, I'll merge

@NthPortal
Copy link
Contributor Author

NthPortal commented Jul 2, 2020

squashed

@SethTisue SethTisue dismissed dwijnand’s stale review July 2, 2020 00:47

requested change was made

@SethTisue SethTisue merged commit dae21fe into scala:2.13.x Jul 2, 2020
@NthPortal NthPortal deleted the topic/12059/PR branch July 2, 2020 07:57
@dwijnand dwijnand changed the title [bug#12059] Make MapView#values return a View Make MapView#values return a View Nov 12, 2020
@SethTisue SethTisue changed the title Make MapView#values return a View Make MapView#values preserve laziness Nov 17, 2020
hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this pull request May 2, 2025
@SethTisue
Merge pull request scala/scala#9090 from NthPortal/topic/12059/PR
99ad0ca 
[bug#12059] Make MapView#values return a View
hamzaremmal pushed a commit to scala/scala3 that referenced this pull request May 7, 2025
@SethTisue
Merge pull request scala/scala#9090 from NthPortal/topic/12059/PR
d02930b 
[bug#12059] Make MapView#values return a View
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julienrf julienrf julienrf left review comments

@dwijnand dwijnand dwijnand left review comments

+1 more reviewer

@neontorrent neontorrent neontorrent approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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.4

Development

Successfully merging this pull request may close these issues.

MapView.values is not lazy and forcing evaluation

6 participants

@NthPortal @julienrf @dwijnand @neontorrent @SethTisue @scala-jenkins