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

iter: use Option::map() in struct Iterater::map() #22958

Merged
merged 1 commit into from
Mar 4, 2015

Conversation

laijs
Copy link

@laijs laijs commented Mar 2, 2015

Signed-off-by: Lai Jiangshan [email protected]

@rust-highfive
Copy link
Collaborator

r? @aturon

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

#[stable(feature = "rust1", since = "1.0.0")]
impl<B, I: Iterator, F> Iterator for Map<I, F> where F: FnMut(I::Item) -> B {
type Item = B;

#[inline]
fn next(&mut self) -> Option<B> {
let next = self.iter.next();
self.do_map(next)
self.iter.next().map(|a| (self.f)(a))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be replaced with self.iter.next().map(self.f)?

Copy link
Author

Choose a reason for hiding this comment

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

self.f can not be moved.
if we have "impl FnMut for &FnMut", then we can use &self.iter.next().map(&self.f)

I'm curious that it is not implemented yet.
I'm also curious that fn(Arg) is not trait Fn(Arg). If it is implemented, it also helps (in other code).

Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be possible with self.iter.next().map(&mut self.f). The fn types are distinct from the Fn traits because fn is a specific type for a function pointer (but implements the Fn trait).

We cannot implement FnMut for &FnMut, but it is implemented for &mut FnMut (as the mutable borrow is required).

Copy link
Author

Choose a reason for hiding this comment

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

Due to the auto implmeting among the Fn, FnMut, FnOnce, we have some constrains.

Among these 3 types: &mut T where T: Fn, &mut T where T:FnMut, &mut T where T:FnOnce, we can only impl one Fn* trait to them, otherwise they will conflict. The only choice is impl FnMut for &mut T where T:FnMut, other choices make no sense.

The same, Among these 3 types: &T where T: Fn, &T where T:FnMut, &T where T:FnOnce, we can only impl one Fn* trait to them, otherwise they will conflict. The best choice is impl Fn for &T where T:Fn, other choices will cover less situation.

Could I implement the above?

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I was under the impression that this was possible when it in fact isn't! I've opened #23015 on the topic.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will leave this and look forward to your implementing.

@alexcrichton
Copy link
Member

@bors: r+ cd65156

@bors
Copy link
Contributor

bors commented Mar 4, 2015

⌛ Testing commit cd65156 with merge ffe3976...

@bors
Copy link
Contributor

bors commented Mar 4, 2015

⌛ Testing commit cd65156 with merge 7003120...

@bors
Copy link
Contributor

bors commented Mar 4, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Mar 4, 2015

@bors
Copy link
Contributor

bors commented Mar 4, 2015

💔 Test failed - auto-win-64-opt

@alexcrichton
Copy link
Member

@bors: retry

@alexcrichton
Copy link
Member

@bors: clean

@bors
Copy link
Contributor

bors commented Mar 4, 2015

⌛ Testing commit cd65156 with merge 6e055c3...

bors added a commit that referenced this pull request Mar 4, 2015
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 4, 2015
@bors bors merged commit cd65156 into rust-lang:master Mar 4, 2015
@Gankra
Copy link
Contributor

Gankra commented Mar 4, 2015

@huonw noted on IRC that the old design was a perf hack from back in the unboxed closures days. @laijs would you be able to confirm that this change didn't regress performance? I'm afraid I'm short on details.

@Gankra
Copy link
Contributor

Gankra commented Mar 4, 2015

Looks like @bluss made the change in #8120; I don't see any reference to perf. Maybe they can confirm, though.

@Gankra
Copy link
Contributor

Gankra commented Mar 4, 2015

Ah no the "hack" was doing the match manually, which that PR didn't change. My git-blame-foo is weak, and I gotta go. Hopefully someone else can figure it out.

@huonw
Copy link
Member

huonw commented Mar 4, 2015

(The code has been that way since it was introduced, in 8bf9fc5.)

@bluss
Copy link
Member

bluss commented Mar 4, 2015

I'm pretty sure I did that .do_map() change just to avoid repeating code.

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.

8 participants