Skip to content

Conversation

@andralex
Copy link
Member

Per title. No need to do decoding if comparing two strings that have the same representation.

@andralex andralex force-pushed the master branch 2 times, most recently from d8a62b0 to 9dee314 Compare February 14, 2015 17:36
Copy link
Contributor

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[]?

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea.

Copy link
Contributor

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.

Copy link
Member Author

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.

@kuettler
Copy link
Contributor

LGTM. As in: I think I understand what you do, why you do it and why it makes sense.

Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

noice

@sinkuu
Copy link
Contributor

sinkuu commented Feb 16, 2015

startsWith already has optimizations like this. I think its logics can be reused for skipOver.

https://github.com/D-Programming-Language/phobos/blob/b55c07c63a76ce56526afb1253ece5cda063849c/std/algorithm/searching.d#L3050

@andralex
Copy link
Member Author

thanks for the review - updated

Copy link
Contributor

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?

Copy link
Member Author

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.

@andralex
Copy link
Member Author

ping

@kuettler
Copy link
Contributor

LGTM.

@WalterBright
Copy link
Member

Auto-merge toggled on

WalterBright added a commit that referenced this pull request Feb 17, 2015
skipOver: no unnecessary decoding
@WalterBright WalterBright merged commit dd00f64 into dlang:master Feb 17, 2015
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.

6 participants