A single iteration method for Option -- iter()

[bluss]

The Option Api uses a unifying simple model, where most methods are called by self, and .as_ref() and .as_mut() are used to modify the access mode.

The Iteration methods still live in the old paradigm, so fn iter(&self), fn mut_iter(&mut self) and fn move_iter(self) all still exist.

I propose simplify this by using the same model as the rest of the API, providing only fn iter(self) -> Item<T>.

Motivation:

• Consistency
• Simplification of the API – iterating Option is rare, so we can afford to collapse these into one method.

EDIT: I changed my mind about this after trying to implement it: Option is ok with 3 iteration methods for consistency with other containers.

[bluss]

By the way, this unifying simple model for the Option Api was proposed and implemented by strcat a.k.a thestinger

[eridius]

I agree, 3 iteration methods on Option is overkill.

[bluss]

I started implementing this – librustc is using it much more than I thought. Most of their uses are .iter() then dereferencing, so these become simpler. However the symmetry with other container iterators (also named .iter()) is lost.

I would now say the method needs a rename if we change its calling convention.

[eridius]

Why does librustc use .iter() on Option so often? That should be a rare thing. The only real reason Option supports iteration is to chain with other iterators; anyone using for x in opt.iter() to test and extract a value from an Option is doing it wrong.

[eridius]

I would now say the method needs a rename if we change its calling convention.

Are you saying this because you’ve decided that iter() is expected to always return a reference? That’s typically true of containers that take arbitrary values, but it’s not true for containers that hold onto known values. For example, Bitv has an iter() method that returns an Iterator<bool>. EnumSet has an iter() that returns the enumeration values. SmallIntMap has iter() that returns (uint, &T) (i.e. the value is still a reference but the key is an immediate value). And so on.

That said, there is the argument that none of these iter() methods consume the receiver. That argument might be enough to warrant sticking with .move_iter() instead, I’m not sure.

[bluss]

The unfinished diff looks like this https://gist.github.com/anonymous/21b9065e522dafa74f78#file-gistfile1-diff-L474

And the highlighted line is an example of where it is now less consistent, because an option was used just like the vec enum field next to it. (This kind of code is quite confusing to edit, since there is no type information for context anywhere close to these lines)

[eridius]

I feel like that highlighted line really should be

slice.map_or(true, |p| walk_pat(&*p, |p| it(p))) &&&

The use of iter() implies it’s a collection of some sort.

[eridius]

Or perhaps even

PatVec(ref before, ref slice, ref after) => {
    before.iter().chain(slice.as_ref().iter()).chain(after.iter())
          .all(|p| walk_pat(&**p, |p| it(p)))
}

[benh]

anyone using for x in opt.iter() to test and extract a value from an Option is doing it wrong.

I still feel like this is the clearest pattern for optionally doing something if the Option isn’t None, when the result of that operation isn’t used further.

[huon]

The only real reason Option supports iteration is to chain with other iterators; anyone using for x in opt.iter() to test and extract a value from an Option is doing it wrong.

I don’t think this is true, just a personal preference of yours. It seems like a perfectly fine way to do it to me.

The use of iter() implies it’s a collection of some sort.

It is a collection.

[eridius]

I still feel like this is the clearest pattern for optionally doing something if the Option isn’t None, when the result of that operation isn’t used further.

My if let RFC provides what I think is a much more elegant solution.

[benh]

Yeah, no argument there.

[reem]

.map() or .and_then() are pretty clean ways of doing the same thing and are arguably much clearer than for x in opt.iter(). Even if opt.is_some() { do_something_with(opt.unwrap()) } has a clearer intent.

[bluss]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.