-
Notifications
You must be signed in to change notification settings - Fork 790
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
Fixing code fixes, part 4 #15551
Fixing code fixes, part 4 #15551
Conversation
vsintegration/tests/FSharp.Editor.Tests/CodeFixes/UseTripleQuotedInterpolationTests.fs
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/CodeFixes/ChangePrefixNegationToInfixSubtraction.fs
Show resolved
Hide resolved
|
||
// in a line like "... x -1 ...", | ||
// squiggly goes for "x", not for "-", hence we search for "-" | ||
let remainingText = $"{sourceText.GetSubText(context.Span.End)}" |
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.
$"{}" should never be a replacement for .ToString() in such case.
It's both worse performance wise as well as worse for readability (reader has to stop and figure out why the hell is string interpolation needed here).
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.
Correction, even worse I guess - the previous code was carefully reading one character at a time from the Roslyn's SourceText.
The new code allocates for the full substring from position till end of the document, even though the algorithm itself only cares about immediate whitespaces after the position.
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.
Readability is debatable, performance - I guess not. I will fix this in the followup soon!
This PR: