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 mutable iteration over arena contents #29

Merged
merged 3 commits into from
Sep 9, 2019
Merged

Conversation

aloucks
Copy link
Contributor

@aloucks aloucks commented Aug 24, 2019

Adds iteration capabilities to Arena. Note that only iter_mut is implemented and there is no iter method. Iteration requires mutable access to the arena, which prevents multiple aliasing on the arena contents. Items are yielded from the iterator in the same order that they are allocated.

Copy link
Collaborator

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few questions below that I'd like to resolve before merging this.

src/lib.rs Outdated
/// // borrow error!
/// *x = 2;
/// ```
pub fn iter_mut(&mut self) -> IterMut<'_, T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub fn iter_mut(&mut self) -> IterMut<'_, T> {
#[inline]
pub fn iter_mut(&mut self) -> IterMut<T> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

}

#[test]
#[cfg(not(miri))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why cfg(not(miri))? I'd strongly prefer to keep all of our unsafe code covered by miri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build timed out on nightly with miri:

https://travis-ci.org/SimonSapin/rust-typed-arena/jobs/576284617

I added another test that does test miri, but iterates over a much smaller range. The other size_hint tests have larger ranges to validate that the min and max bounds are staying within reasonable values.


impl<'a, T> Iterator for IterMut<'a, T> {
type Item = &'a mut T;
fn next(&mut self) -> Option<&'a mut T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a lot of relatively hairy code, and there's a lot more here than I was expecting when I opened the PR.

What I was expecting was that this method would be defined in terms of a couple of inner: std::slice::IterMut<'a, T>s (one for the chunk list and another for the chunk currently being iterated over). I suspect that would lead to smaller, easier to follow code. Did you investigate this approach at all? Is there a reason you didn't pursue it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you would still need to track state to know whether to pull from the rest vec of chunks or the current chunk. I think the primary difference would be Rest { index, iter } and Current { iter } instead of the raw indices. I'll explore using inner iterators later this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a quick look this evening and I'm not sure that inner iterators would lead to less gnarly code in this case.

struct ChunkList<T> {
    current: Vec<T>,
    rest: Vec<Vec<T>>,
}

We have three indices to track, currently represented by the ChunkListPosition iterator state:

enum ChunkListPosition {
    Rest { index: usize, inner_index: usize },
    Current { index: usize },
}

We'd have to introduce a lot of Optional in the state tracking and I think the lifetime transmutes would be more difficult to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fitzgen I've updated this to use inner iterators rather than the raw indices.

Copy link
Collaborator

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great -- I personally find this much easier to follow now, thanks for the update and thanks for your patience.

@fitzgen fitzgen merged commit e3a693a into thomcc:master Sep 9, 2019
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