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

Off by one index in Memchr iterator #13

Closed
matt2xu opened this issue Jan 16, 2017 · 6 comments
Closed

Off by one index in Memchr iterator #13

matt2xu opened this issue Jan 16, 2017 · 6 comments

Comments

@matt2xu
Copy link

matt2xu commented Jan 16, 2017

It seems surprising to me that when using the Memchr iterator, the position that is returned when calling .next() is the position of the needle + 1. Looking at the tests of the crate, this is expected behavior. Could you please explain why it is this way? I could then make a PR with additional documentation and an example to show usage of the Memchr (and Memchr2/3) iterator.

@BurntSushi
Copy link
Owner

Uh... That seems surprising to me and sounds like a bug. cc @cholcombe973

@cholcombe973
Copy link
Contributor

Yeah I seem to recall the problem was if I didn't return the needle position plus one it would get stuck in a infinite loop.

@BurntSushi
Copy link
Owner

sigh OK. Returning the index + 1 doesn't seem right to me at all. I really wish I caught that.

This has to be fixed. The question now is whether this requires a 2.0.0 release. It probably does.

@BurntSushi BurntSushi added the bug label Jan 16, 2017
@cholcombe973
Copy link
Contributor

It seemed right at the time :-/. I think I was using it to split strings.

@matt2xu
Copy link
Author

matt2xu commented Feb 27, 2017

Any update on this? It would be nice to be zero-based IMO.

@BurntSushi
Copy link
Owner

Fixed in #18. Note that we decided it would be best to do a memchr 2 release for this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants