Skip to content

Improve printing of strings#22945

Merged
som-snytt merged 4 commits intoscala:mainfrom
som-snytt:port/sd-878-const
Oct 22, 2025
Merged

Improve printing of strings#22945
som-snytt merged 4 commits intoscala:mainfrom
som-snytt:port/sd-878-const

Conversation

@som-snytt
Copy link
Copy Markdown
Contributor

@som-snytt som-snytt commented Apr 9, 2025 •
edited
Loading

Ports scala/scala#10897

It's a port of a Scala 2 support ticket, which means it is a performance improvement worth paying for. The code is not shared with Scala 2 but is similar in principle.

The bottleneck is string.flatMap(escapedChar). At the urging of the reviewer, reduced code duplication between PlainPrinter and SourceCode by sticking it in util.Chars.

@Mikhail42
Copy link
Copy Markdown

Mikhail42 commented Apr 28, 2025 •
edited
Loading

It would be nice to add at least a few tests. I've not found them here (https://github.com/scala/scala3/tree/main/compiler/test/dotty/tools/dotc/printing)

@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Apr 29, 2025

I would do something about the code duplication before undrafting.

Everything breaks if strings are broken, and on scala 2 it was also a zinc dependency. A useful test would be a scalacheck, as there are many boundary conditions. An existing test broke because it noticed whether Unicode escapes were lowercase. (I did not argue with the test.)

@som-snytt som-snytt marked this pull request as ready for review September 18, 2025 12:07
@Gedochao Gedochao requested a review from noti0na1 September 18, 2025 13:24
noti0na1
noti0na1 requested changes Oct 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

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

Sorry for so long to back to this PR. Other than the code duplication, the changes look good to me.

@som-snytt som-snytt force-pushed the port/sd-878-const branch 2 times, most recently from 605e569 to f419085 Compare October 15, 2025 19:55
@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Oct 15, 2025

Inlined quadNibble just for the pleasure of watching it unroll. It would be nice to invest in a jmh test to verify that it doesn't have unintended consequences. I misplaced the test for Scala 2 that demonstrated it.

noti0na1
noti0na1 approved these changes Oct 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

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

Let's merge after CI becomes green!

@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Oct 22, 2025

rebased

@som-snytt som-snytt merged commit d0ae16e into scala:main Oct 22, 2025
51 checks passed
@som-snytt som-snytt deleted the port/sd-878-const branch October 22, 2025 20:10
@WojciechMazur WojciechMazur added this to the 3.8.0 milestone Oct 28, 2025
@tgodzik tgodzik mentioned this pull request Nov 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@noti0na1 noti0na1 noti0na1 approved these changes

Assignees

@noti0na1 noti0na1

Labels

None yet

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

6 participants

@som-snytt @Mikhail42 @noti0na1 @tgodzik @Kordyjan @WojciechMazur