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

add into_lending_refs and into_lending_refs_mut #18

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

ratmice
Copy link
Collaborator

@ratmice ratmice commented Nov 25, 2023

Here is a first stab at an abstraction similar to the one we discussed in #14.
I wasn't able to figure out a way to implement it exactly as you had mentioned.
In there you had mentioned it being a method on LendingIterator, which is where I ran into problems.
So for the time being these both take a Iterator instead and subsequently lend references, and mut references respectively to the iterator's item.

The issue I ran into implementing it on LendingIterator is as follows, in order to hold an Item<'i> we end up bringing the lifetime up into the struct, so far so good.

pub struct LendingRefs<'i, I>
where
   I: LendingIterator + 'i,
{
    item: Option<I::Item<'i>>,
    iter: I,
}

But once it comes time to implement the LendingIterator trait, I couldn't figure out a way to implement the trait, such that it both lives long enough, and also matches the trait. Because we end up having to bind the lifetimes to the self and returned item.

I think the names I'd chosen into_lending_refs and into_lending_refs_mut are a bit long and verbose.
So I'm not all that partial to them, but I do find them clear.

If you have any ideas on how to implement it for LendingIterator instead, I'd be happy to keep trying but I myself ran out of ideas there.

Edit: One idea to try and work around this lifetime error is leveraging MaybeUninit, will try that tomorrow.

@ratmice ratmice marked this pull request as draft November 25, 2023 19:27
@Crazytieguy
Copy link
Owner

Crazytieguy commented Nov 26, 2023

I see how that can be tricky, I'll give it a shot as well later. If we can't do it your version looks great 🙂

Regarding the names - I think it makes it sound like the refs are lending something, maybe lend_refs and lend_refs_mut? Definitely not sure myself

@ratmice
Copy link
Collaborator Author

ratmice commented Nov 26, 2023

I wasn't able to get it working with MaybeUninit either (same problem), I think it's just the compiler isn't able to infer an outlives lifetime bounds between 'i and the lifetime of the fn next(&mut self), and we can't actually add one without making things LendingIterator<'i>, and adding that outlives bounds to the trait. Because you'd think it should be able to coerce the longer lifetime to a shorter one.

lend_refs and lend_refs_mut sound fine to me, if we end up going this route.

@Crazytieguy
Copy link
Owner

I wasn't able to get the LendingIterator version to work, so I'm ready to merge your code when you are 👍

Also - I'm going to make you a contributor so you can work directly in the repo, seems more convenient

These both take a `Iterator` and subsequently lend references, and mut
references respectively to the iterator's item.
@ratmice ratmice marked this pull request as ready for review December 1, 2023 22:27
@ratmice
Copy link
Collaborator Author

ratmice commented Dec 1, 2023

I went ahead an unmarked it as a draft, and rebased the change that renamed those functions out.
I'll leave it for a day or so, in case we come up with any last minute ideas.

@ratmice
Copy link
Collaborator Author

ratmice commented Dec 5, 2023

FWIW, I spent some time trying to see if the modifying the trait works,
while I can get a modified trait to compile the iterator in question, compiling the rest of the library with
that trait is another thing entirely, as it seems to break the rest of the library pretty badly.

I do find the 'a: 'i bounds it requires weird, and don't fully understand exactly how it comes up with that bound.

trait LendingIterator<'i>: Sized {
    type Item<'a>
    where
        Self: 'a, 'a: 'i;
    fn next<'a>(&'a mut self) -> Option<Self::Item<'a>>;
}

struct LendRefs<'i, I: LendingIterator<'i> + 'i> {
    item: Option<I::Item<'i>>,
    iter: I,
}

impl<'i, I: LendingIterator<'i> + 'i> LendingIterator<'i> for LendRefs<'i, I> {
    type Item<'a> = &'a I::Item<'a> where Self: 'a, 'a: 'i;
    fn next<'a>(&'a mut self) -> Option<Self::Item<'a>> {
        self.item = self.iter.next();
        self.item.as_ref()
    }
}

@Crazytieguy
Copy link
Owner

Huh, that's pretty cool. Does it actually compile when you test it? Don't know if it's worth it though

@ratmice
Copy link
Collaborator Author

ratmice commented Dec 5, 2023

I didn't really get enough working to actually manage to write any tests. a good portion compiled, adaptors like Chain being the most complex, Take, etc. But anything taking an F type parameter, or trying to actually loop over the iterator fell apart, such as for_each, being the simplest but also Map, Filter, etc. So it seemed somewhat hopeless to continue to try.

137 |     fn for_each<'a, F>(mut self, mut f: F)
    |                 -- lifetime `'a` defined here
...
143 |         while let Some(item) = self.next() {
    |                                ^^^^-------
    |                                |
    |                                `self` was mutably borrowed here in the previous iteration of the loop
    |                                argument requires that `self` is borrowed for `'a`

@Crazytieguy
Copy link
Owner

Thanks for trying! I'll merge and publish the into version

@Crazytieguy Crazytieguy merged commit 74821af into Crazytieguy:master Dec 5, 2023
1 check passed
@ratmice ratmice deleted the lending_refs branch December 5, 2023 09:00
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.

2 participants