Added str::char_offset_iter() and str::rev_char_offset_iter()

[Kimundi]

Also renamed bytes_iter to byte_iter to match other iterators

[alexcrichton]

You don't need to re-implement the StrCharIterator, methods within the same package can read the private fields of that struct (index)

[alexcrichton]

While we're renaming things, could you also drop the Str prefix on all the iterators? It looks a little silly to import any of them as str::StrXXXIterator.

If you'd rather not do that here though, I'll r+ with the changes.

[bluss]

This iterator should be bidirectional. So far none of the str iterators are.

[alexcrichton]

Along the same lines as these should be bi-directional, why can't this just be a VecIterator with some various bounds?

[bluss]

It's a vec iterator composed with a dereference from &u8 to u8

[alexcrichton]

Oh good point... Although then it seems like there should be:

pub type ByteIterator = TransformIterator<..., ..., VecIterator<..., ...>>;

And then it just returns one of those.

[Kimundi]

Whoa, lots of notes

• The realization that they could be bidirectional came after I already finished this patch. I could change that, but I won't have time for that in the next 2 days, so maybe just r+ for now.
• Likewise for the Str prefix, I can remove it, but won't have time for it right now.
• Duplicating the StrCharIterator was so that the method inference doesn't need to be told which of the two possible implementations it has to choose.

If I where to reuse the same struct, this:

s.iter()
s.char_offset_iter()

would turn into this:

s.iter::<char>()
s.iter::<(char, uint)>()

[huonw]

@Kimundi fwiw, when I was thinking about implementing this, I was going to write the (uint, char) iterator, and convert (what is now called) StrCharIterator to be:

type StrCharIterator<'self> = MapIterator<'self, (uint, char), char, StrCharOffsetIterator<'self>>;

fn iter() -> StrCharIterator<'self> {
    self.char_offset_iter().transform(|(_, c)| c)
}

(Similar to how AnyLineIterator and WordIterator work.)

[bluss]

Writing the iterators with MapIterator sounds fine, the adaptor will preserve the underlying properties like bidirectionality.

[alexcrichton]

@Kimundi, I'll open an issue for the Str prefix, and I'd be perfectly happy with @huonw's suggestion

[Kimundi]

@alexcrichton: Sounds good, it will have to wait a day more though :)

[Kimundi]

I'm running the testsuite right now to catch remaining bugs, but here is what I've done so far:

• Reduced 6 structs with 6 Iterator implementations (3 distinct Iterators in 2 directions) down to 1 struct with an Iterator and DoubleEndedIterator implemenetation. The remaining 5 are now expressed as typedefs for chained Iterator Adapters.
• Moved clunky each_split_within out of str. This function is only used in extra::getops, and even there in a wrong way (byte offset are being equaled to character widths)
• Did a bunch of whitespace and style fixups in str

[huonw]

Could you add a note about why this was moved here, and note the fact that it's incorrect wrt multibyte codepoints.

[Kimundi]

I'm not sure if both of these things need to be noted here. It's there because the function above it needs it, and
it's working correctly, the function above just misuses it, which already has an Issue number.

[huonw]

Oh, ok, I misinterpreted it; this seems fine then.

[Kimundi]

That said, I could add a line like "Was moved here from std::str because this is the only place that uses it, and because it was to specific for a general sting function."

[bluss]

it's beautiful (my comments were addressed, too)

[Kimundi]

Someone on irc raised an interesting point: Just having a CharOffsetIterator seems kinda arbitrary, seeing how there are other information calculated by the Iterator that might be just as useful, like character as utf8 slice, character byte length etc.

How about replacing the CharOffsetIterator by a CharContextIterator that instead of a (uint, char) yields a struct:

struct CharContext {
    chr: char,
    chr_slice: &str
    byte_offset: uint,
    byte_len: uint,
}

Anyway, that would be an future patch.

[bluss]

An iterator producing only byte_offset and chr_slice: &str would be the fastest, since it doesn't even have to decode the char, only look at the first byte for width. The byte_len info is already included as the length of the chr_slice.

[huonw]

Everything other than byte_offset is easily computable from other information (e.g. s.slice_from(byte_offset), s.char_offset_iter().enumerate(), len_utf8_bytes(c)), whereas the offset requires having a running sum across iterations. So I'd prefer keeping it as small as possible.

[bluss]

I'm sorry for the conflicts with pull #8090. Should be mostly MapIterator → Map