Discuss pitfalls of stateful closures with Map

[steveklabnik]

Fixes #30632

I'm not sure if this explanation is good enough. If it is, I will add it to filter as well.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[bluss]

.rev()

[steveklabnik]

ugh, i fixed it in my test but not when I put it over here. was too worried about the words

[nikomatsakis]

I think I would find the example clearer if you included the items being iterated over, and they were not the numbers 1, 2, and 3. For example:

vec!['a', 'b', 'c'].into_iter().map(...).rev();

will produce ('c', 1), ('b', 2), ('a', 3), right?

[steveklabnik]

Ah, this is a good point. Yes, something like this would be much better.

[bluss]

Maybe it can instead underline that the map's closure is called on elements in the order they are "extracted" from the iterator (i.e. if you mix next/next_back or call just next_back etc). Saying that the iteration is not backwards is too confusable with how map() takes elements from the underlying iterator (which is not affected).

[steveklabnik]

Okay, so i pushed a new commit which actually fails. The weird thing about doing what @nikomatsakis says is that it does illustrate what's happening better, which, uh, makes it feel not weird. And so now this documentation feels strange, because it feels like it's saying "hey this is so counterintuitive"... but it's not.

So, I'm kinda almost leaning towards not writing something about this. Yes, writing weird code might be a bit confusing, but you're also writing odd code? I dunno.

[frewsxcv]

Minor nit: using the variable name a when 'a' is being iterated over is slightly confusing

[steveklabnik]

agreed

[nikomatsakis]

@steveklabnik planning to update?

[steveklabnik]

Yes, later today.

On Jan 29, 2016, 15:20 -0500, Niko [email protected], wrote:

@steveklabnik(https://github.com/steveklabnik)planning to update?

—
Reply to this email directly orview it on GitHub(#31220 (comment)).

[steveklabnik]

@nikomatsakis I have updated it to your example, with one or two minor wording/format tweaks.

I was trying to think of a way to get away from 'printing' examples, as I don't like them as much as assertions, since printing won't be automatically tested, but assertions end up just looking awkward in examples like this, so I don't think I can do much better.

[nikomatsakis]

@steveklabnik

I was trying to think of a way to get away from 'printing' examples, as I don't like them as much as assertions, since printing won't be automatically tested, but assertions end up just looking awkward in examples like this, so I don't think I can do much better.

I see. Makes sense. But I agree that here printing is natural -- also, this feels like an example that is unlikely to bitrot.

[nikomatsakis]

Note travis failure:

/home/travis/build/rust-lang/rust/src/libcore/iter.rs:3299: trailing whitespace

Seems legit?

The only other nit is that maybe it'd be better to separate the two examples a bit. e.g.,

+/// However, if you call `.rev()` to iterate in the other direction, it
+/// will only be 'backwards' if your closure has no state. As an example,
+/// consider this loop, which iterates *forward* and uses `map` to
+/// track a mutable counter. This will print out `"('a', 1), ('b', 2), ('c', 3)"`.
+///
+/// ```rust
+/// let mut c = 0;
+///
+/// for pair in vec!['a', 'b', 'c'].into_iter()
+///                                .map(|letter| { c += 1; (letter, c) }) {
+///     println!("{:?}", pair);
+/// }
+/// ```
+///
+/// Now consider this twist on the previous example, where we add
+/// a call to `rev`. This version will print `('c', 1), ('b', 2), ('a', 3)`.
+/// Note that the letters are reversed, but the values of the counter
+/// still go in order. This is because `map` is still being called lazilly
+/// on each item, but we are popping items off the back of the vector now,
+/// instead of shifting them from the front.
+///
+/// ```
+/// let mut c = 0;
+///
+/// for pair in vec!['a', 'b', 'c'].into_iter()
+///                                .map(|letter| { c += 1; (letter, c) })
+///                                .rev() {
+///     println!("{:?}", pair);
+/// }
+/// ```

[nikomatsakis]

r=me either to the current commit or a version like what I suggested, once travis failure is resolved.

[steveklabnik]

I was thinking of the same thing. Tidy passes, and I changed to that.

@bors: r=nikomatsakis rollup

[bors]

📌 Commit 6f85be0 has been approved by nikomatsakis

[steveklabnik]

Okay. This time I checked that both the tests and make tidy passed, and re-worded some stuff a bit.

[nikomatsakis]

This is a work of art.

@bors r+ rollup

[bors]

📌 Commit 7deb057 has been approved by nikomatsakis

[steveklabnik]

❤️