-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve printing of strings #22945
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
base: main
Are you sure you want to change the base?
Improve printing of strings #22945
Conversation
98a80fa
to
9f19b05
Compare
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) |
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.) |
9f19b05
to
e0c617f
Compare
There was a problem hiding this 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.
else if quoted then "\"" + text + "\"" | ||
else text | ||
|
||
private def escapedChar(b: StringBuilder, c: Char): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the functions to compiler/src/dotty/tools/dotc/util/Chars.scala
to avoid duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can!
else text | ||
|
||
private def escapedChar(b: StringBuilder, c: Char): Unit = | ||
def quadNibble(b: StringBuilder, x: Int, i: Int): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the parameter b
here, since it is not changed.
7dac1c6
to
605e569
Compare
605e569
to
f419085
Compare
Inlined |
Ports scala/scala#10897
Why was the ticket worth tackling?
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.
How I fixed it
The bottleneck is
string.flatMap(escapedChar)
. At the urging of the reviewer, reduced code duplication betweenPlainPrinter
andSourceCode
by sticking it inutil.Chars
.Why is this PR worth reviewing?
It would be more compelling with a jmh demonstration, aside from correctness. This PR is like grading homework, thankless but instructional.