-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Simplify normalizeSlashes #50154
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
Simplify normalizeSlashes #50154
Conversation
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at ec51f12. You can monitor the build here. Update: The results are in! |
} | ||
backslashRegExp.lastIndex = index; // prime regex with known position | ||
return path.replace(backslashRegExp, directorySeparator); | ||
return path.indexOf("\\") !== -1 |
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.
Alternatively, use ===
and flip the branches.
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.
Yeah, I considered that, but opted to match the other functions that do regex.test(s) ? s.replace(regex, "...") : s
. From my testing it was the same, I think.
@jakebailey Here they are:
CompilerComparison Report - main..50154
System
Hosts
Scenarios
TSServerComparison Report - main..50154
System
Hosts
Scenarios
Developer Information: |
Uh hmm, I guess it's slower? Am I misreading the results? |
Looks like noise to me - I'd run it again. |
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at ec51f12. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..50154
System
Hosts
Scenarios
TSServerComparison Report - main..50154
System
Hosts
Scenarios
Developer Information: |
I'd call that a wash. The change still has hygiene value, but I might wait until 4.9. |
Ideally you'll see no change because the perf tests all run on Linux and theoretically should have no |
You're right. I guess I don't know what to do about that one. |
I think we actually have a bug about that: Linux paths can contain |
Yep, #44174. Though I'm a little apprehensive about that one just because of cross-platform configuration. |
I'll wait for us to cut 4.8 before merging this, just in case. |
It turns out that
replace
totally ignoreslastIndex
. It's faster to not set the property, and then (seemingly) a little faster still to use a conditional (it is at least consistent with other functions like this one).