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

Fuse impls expose use of specialization (default fn) #70796

Closed
Centril opened this issue Apr 5, 2020 · 10 comments · Fixed by #70910
Closed

Fuse impls expose use of specialization (default fn) #70796

Centril opened this issue Apr 5, 2020 · 10 comments · Fixed by #70910
Assignees
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Apr 5, 2020

Context: #70750 (comment)

We have some uses of default fn in src/libcore/adapters/fuse.rs which allow users to specialize those functions. Instead, these default fns should be moved into internal (private) traits so that the specialization isn't exposed.

This issue has been assigned to @rakshith-ravi via this comment.

@Centril Centril added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-iterators Area: Iterators labels Apr 5, 2020
@rakshith-ravi
Copy link
Contributor

rakshith-ravi commented Apr 6, 2020

Supp. Can I take this up?

Oh also, the file is in src/libcore/iter/adapters/fuse.rs not src/libcore/adapters/fuse.rs. Had me confused for a while, but picked it up from context of the other PR, just saying.

@cuviper
Copy link
Member

cuviper commented Apr 6, 2020

@rustbot assign @rakshith-ravi

@rustbot rustbot self-assigned this Apr 6, 2020
@cuviper
Copy link
Member

cuviper commented Apr 6, 2020

I fear what the extra indirection will do to #70332, but I guess we'll just have to see...

@rakshith-ravi
Copy link
Contributor

Alright. I'll go through that as well and suggest changes there, in case I can think of any.

Unlike my last PR, this time, I'd like to give this a shot without any mentoring. However, if I get stuck somewhere, I might request for help. Bear with me please.

Anyways, it's a little too late for me here (IST). Will get on it first thing tomorrow morning. Thanks

@cuviper
Copy link
Member

cuviper commented Apr 6, 2020

It's also possible that could work out better if there's a way for Chain to directly reach the non-specialized version, rather than shimming its own Defuse to avoid FusedIterator effects.

@rakshith-ravi
Copy link
Contributor

So if I understand the situation correctly, we need to have Iterator inherit from IteratorInternal or something like that, which allows for specialization and perform all our specialized work on that, while not exposing the default fns to anything outside the crate, yes?

This will also mean that Iterator will provide a blanket implementation that uses the IteratorInternal's implementation.

@rakshith-ravi
Copy link
Contributor

It's also possible that could work out better if there's a way for Chain to directly reach the non-specialized version, rather than shimming its own Defuse to avoid FusedIterator effects.

I don't think I entirely understand what you're saying. Could you help me clarify that please?

@cuviper
Copy link
Member

cuviper commented Apr 7, 2020

We definitely don't want to change the Iterator trait, if that's what you mean. I would look at Zip and its ZipImpl to see how that's privately specialized.

It's also possible that could work out better if there's a way for Chain to directly reach the non-specialized version, rather than shimming its own Defuse to avoid FusedIterator effects.

I don't think I entirely understand what you're saying. Could you help me clarify that please?

My thought was that we could make #70332 use some internal part of your work, e.g. FuseImpl, to bypass some of the nested calls. But now I'm also trying another Chain independent of Fuse in #70896 which I think is promising...

@rakshith-ravi
Copy link
Contributor

Alrighty. Let me know if #70322 requires something specific so that I can implement it accordingly. Would be happy to dive deep into this.

@rakshith-ravi
Copy link
Contributor

We definitely don't want to change the Iterator trait, if that's what you mean. I would look at Zip and its ZipImpl to see how that's privately specialized.

Yup, an implementation similar to ZipImpl is exactly what I meant. I'll get right on it then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants