Skip to content
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

[misc] speedup month & era locale handling #5581

Closed
wants to merge 1 commit into from

Conversation

Alanscut
Copy link
Contributor

@Alanscut Alanscut commented Jun 4, 2020

summary

Optimize computeErasParse and computeMonthsParse , reduce unnecessary regular escaping and loop traversal.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 88.524% when pulling 3afc8d5 on Alanscut:optimizeCode into e3c6790 on moment:develop.

@Alanscut Alanscut changed the title Optimize computeErasParse and computeMonthsParse [bugfix]Optimize computeErasParse and computeMonthsParse Jun 4, 2020
@marwahaha marwahaha changed the title [bugfix]Optimize computeErasParse and computeMonthsParse [bugfix] Optimize computeErasParse and computeMonthsParse Jun 9, 2020
}
// Sorting makes sure if one month (or abbr) is a prefix of another it
// will match the longer piece.
shortPieces.sort(cmpLenRev);
longPieces.sort(cmpLenRev);
mixedPieces.sort(cmpLenRev);
for (i = 0; i < 12; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could change functionality, correct?

It's first a sort, then escape. Your change escapes, then sorts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regexEscape function may change the length of shortP, longP, and mixedP, leading a wrong sorting, so I changed the order of escapes and sorts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you ever want to sort before you escape the regex?
Does it make sense to test this behavior?

@ichernev
Copy link
Contributor

Merged in a3babc5

@ichernev ichernev changed the title [bugfix] Optimize computeErasParse and computeMonthsParse [feature] Optimize computeErasParse and computeMonthsParse Dec 23, 2023
@ichernev ichernev closed this Dec 23, 2023
ichernev added a commit that referenced this pull request Dec 23, 2023
[feature] Optimize computeErasParse and computeMonthsParse
@ichernev
Copy link
Contributor

I tweaked a bit the eras code, was double-escaping the regex for the mixed case.

@ichernev ichernev changed the title [feature] Optimize computeErasParse and computeMonthsParse [misc] speedup month & era locale handling Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants