-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Revert Replace ASCII control chars with Unicode Control Pictures #128179
Conversation
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
@bors rollup=never |
@bors r+ I'd like to see this code re-landed in a way that's more efficient, since the improvement to diagnostics is quite nice. This is worth probably a spin-off discussion about how diagnostics formatting code affects the good path in the case of lints and warnings. |
I'm honestly a bit shocked that it had a perf regression (as it only affected rendering...) Edit: I'm assuming it is some second order effect on |
See the discussion at #128169 (comment) for more info, if you haven't already |
@compiler-errors thanks, catching up |
I suspect that the culprit is https://github.com/rust-lang/rust/pull/128200/files#diff-a35029c589a6b5b002ecf598efb24d174232e719ecbc6f741a58eee8509cd6a5L2612-L2614 It wasn't noticeable before because the list was short enough, but big-O likely dominates now over constant times. |
@bors r- Let's give this a tiny bit of time to let Esteban fix the perf w/o a full revert. |
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing in favor of #128200 which gains much of the performance back and should land soon. |
reverts #127528 as that was a perf regression
r? @estebank