Skip to content

Add Lines, Chars and CharsMut iterators #31

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

Closed
wants to merge 14 commits into from

Conversation

burtonageo
Copy link
Contributor

Adds iterators over lines, chars and a mutable iterator over characters, and adds tests as well.

Copy link
Owner

@tomprogrammer tomprogrammer left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I've left some comments and like to hear your opinion. Also I've requested two little changes but then it's ready for squashing and merging.

/// An iterator over the lines of the internal character array.
#[derive(Debug)]
pub struct Lines<'a> {
// TODO: should this use `core::slice::Split` internally?
Copy link
Owner

Choose a reason for hiding this comment

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

core::slice::Split only allows a single byte at a time to be checked. We would check for \n and then see if there was a \r right before I guess. In this case I think using the memchr crate would give a nice speedup.

Do you like to try implementing this? If not, no problem, I'll just open an issue to not forget it.

src/ascii_str.rs Outdated
Some(i) => i,
None => return None
};
let line: &AsciiStr = From::from(&self.string[curr_idx..next_idx]);
Copy link
Owner

Choose a reason for hiding this comment

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

self.string is already of type &AsciiStr which makes the From::from conversion redundant. You can simply omit it.


#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
(self.len(), Some(self.len()))
Copy link
Owner

Choose a reason for hiding this comment

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

I guess the compiler is smart enough to optimize this such that the entire string is only once iterated over instead of twice. I will check this, but it's only a minor observation.

src/ascii_str.rs Outdated
if i == 0 {
*achar = AsciiChar::H;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

if i == 0 makes the for-loop redundant. Maybe rewrite this as:

*ascii.chars_mut().next().unwrap() = AsciiChar::H;

@burtonageo
Copy link
Contributor Author

Thanks for your replies. Unfortunately, I've realised that the <Lines as ExactSizeIterator>::len method is broken, and I've added tests which demonstrate that. I'll need to look at that implementation some more (or perhaps yank it and make that a future pull request).

@burtonageo
Copy link
Contributor Author

I think I'll close this pr, and open a new one with the more focused changes (aka Chars, CharsMut and lines without the len stuff). Then those can be added as future improvements.

@burtonageo burtonageo closed this Feb 7, 2017
@burtonageo burtonageo mentioned this pull request Feb 7, 2017
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