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

List(...) optimization to avoid intermediate array #17166

Merged
merged 7 commits into from
Nov 28, 2023
Merged

List(...) optimization to avoid intermediate array #17166

merged 7 commits into from
Nov 28, 2023

Conversation

KuceraMartin
Copy link
Contributor

No description provided.

@KuceraMartin KuceraMartin marked this pull request as draft March 28, 2023 17:11
@KuceraMartin
Copy link
Contributor Author

KuceraMartin commented Mar 28, 2023 •
edited by dwijnand
Loading

todo:

  • fix lists of primitive types
  • limit for nested lists
  • more tests

@mbovel
Copy link
Member

mbovel commented May 4, 2023

Hi @KuceraMartin! Would you like to continue working on this PR during the next issue spree? Do you think the TODOs left are easy and could be interesting for beginners to follow along?

@KuceraMartin
Copy link
Contributor Author

Hi @KuceraMartin! Would you like to continue working on this PR during the next issue spree? Do you think the TODOs left are easy and could be interesting for beginners to follow along?

Hi @mbovel, actually I'm not sure if this is suitable for another spree.

  1. The first todo might be interesting but maybe too easy?
  2. For the second one I'm not sure—if I remember correctly, the problem with the Scala 2 implementation was that it was calling super.transform recursively to handle the nesting, which we cannot do with the Scala 3 mini phases? But then I think @smarter mentioned he wasn't sure if we should re-implement this nested limit at all.
  3. Writing more tests is probably not very interesting.

I was planning to finish the PR by myself. If it doesn't get picked for the spree, I should have time to do it next week.

@dwijnand dwijnand force-pushed the list-avoid-intermediate-array branch from f940d92 to b09fe7d Compare November 9, 2023 12:43
@dwijnand dwijnand force-pushed the list-avoid-intermediate-array branch from ac14b5d to 2ac7c1c Compare November 15, 2023 22:52
@dwijnand dwijnand marked this pull request as ready for review November 16, 2023 00:51
@dwijnand dwijnand changed the title [WIP] List(...) optimization to avoid intermediate array List(...) optimization to avoid intermediate array Nov 16, 2023
@dwijnand dwijnand assigned KuceraMartin and unassigned mbovel Nov 16, 2023
@dwijnand
Copy link
Member

@KuceraMartin ticked off your TODOs, in my judgement. Could you have a look and let me know if you're happy with this landing?

@nicolasstucki nicolasstucki self-assigned this Nov 20, 2023
@nicolasstucki nicolasstucki merged commit 841bbd4 into scala:main Nov 28, 2023
19 checks passed
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
@WojciechMazur WojciechMazur mentioned this pull request Jun 25, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jun 26, 2024
@WojciechMazur
Backport "List(...) optimization to avoid intermediate array" to LTS (#…
ff6bebe 
…20779)

Backports #17166 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@som-snytt som-snytt mentioned this pull request Oct 2, 2024
Merged
sjrd added a commit that referenced this pull request Oct 2, 2024
@sjrd
Always rewrite empty List() to Nil (#21689)
c332766 
`List[String]()` was OK but not `List()` with `Nothing` inferred, which
is how people write it.

Follow-up to #17166

Noticed at scala/scala#10876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwijnand dwijnand dwijnand left review comments

@nicolasstucki nicolasstucki nicolasstucki approved these changes

Assignees

@KuceraMartin KuceraMartin

@nicolasstucki nicolasstucki

Labels
None yet
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@KuceraMartin @mbovel @dwijnand @nicolasstucki @Kordyjan @Decel