-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Preserve newlines from original source when printing nodes from TextChanges #36688
Conversation
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 0ea8d79. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
Ouch |
@typescript-bot perf test this please and thank you |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 6ff9c2b. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 6ff9c2b. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot perf test this or else |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 6ff9c2b. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
@rbuckton this is still in-progress, but would you mind taking a look and seeing if anything jumps out at you perf-wise? For a |
@typescript-bot perf test this again please |
Heya @andrewbranch, I've started to run the perf test suite on this PR at e3ef427. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at e535e27. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at e7c2b28. You can monitor the build here. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at e99d833. You can monitor the build here. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
@typescript-bot perf test this again please |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 6daa27e. You can monitor the build here. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..36688
System
Hosts
Scenarios
|
Results are a little scattered but seem to be plausibly within ~1% difference |
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.
Code seems more straightforward than the last time I read this. The changes to the existing tests look good to me.
Fixes #27294
I feel like some of this logic looks kind of ugly, but the perf impact for normal compiler runs is down to nothing, so I’m ready to get some fresh eyes on this.
Note that this is still dependent on the emitter actually checking for source newlines between tokens, which it was already pretty good about, but there may still be some cases where newlines get removed. Additionally, the formatter runs after TextChanges, and so any formatter rules that add or remove newlines will take priority. But, this more than addresses the example given in #27294, which was just about newlines between full statements/declarations. Generally, preserving newlines in anything that’s a list works quite well, and I think that’s likely to be what people care most about (followed by chained property access and nested binary expressions, which are also tested here).