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

max = Math.min(..) not needed, we know len < olen #1746

Closed
wants to merge 1 commit into from

Conversation

oyejorge
Copy link
Contributor

No description provided.

@seven-phases-max
Copy link
Member

Btw., I would not bother with optimizations for this particular "matching" code as we may decide to rewrite it from scratch to support #1539 and/or #1196 and similar (see for example #1624). On the other hand an optimization for current version can't harm of course so I don't not mind if this goes in.

@oyejorge
Copy link
Contributor Author

Looks like this change was made in another recent commit

@oyejorge oyejorge closed this Dec 20, 2013
@seven-phases-max
Copy link
Member

@oyejorge That other commit had the same redudant min call, so if you thinks this is important you could make a new PR.
I still treat this as a temporary code (because merged #1624 does not implement the feature in its "ideal/complete" way, I mentioned problems left in the PR comments) so I think eventually it is going to be reworked. Either way thank you for pointing that line out.

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.

2 participants