-
-
Notifications
You must be signed in to change notification settings - Fork 749
skipOver: no unnecessary decoding #2986
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
Conversation
d8a62b0 to
9dee314
Compare
std/algorithm/searching.d
Outdated
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 this be extended to all T[]?
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.
I'm thinking the generated code should be already good for T[]s, isn't it?
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.
No idea.
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.
Your string version would be better for things like int[] and float[] as well since I believe the meat of the function would be vectorized. Probably best to extend.
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.
OK, good idea. Will do.
|
LGTM. As in: I think I understand what you do, why you do it and why it makes sense. |
std/algorithm/searching.d
Outdated
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.
The other, non-string version of the function doesn't have this length check fast path. Could you add it for when both ranges have lengths?
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.
noice
|
|
|
thanks for the review - updated |
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.
Is the overload needed instead of a default pred?
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.
I'm leaving it like that pending future improvements based on equality.
|
ping |
|
LGTM. |
|
Auto-merge toggled on |
skipOver: no unnecessary decoding
Per title. No need to do decoding if comparing two strings that have the same representation.