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 iter_allocated_chunks_raw() #127

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Add iter_allocated_chunks_raw() #127

merged 4 commits into from
Oct 19, 2021

Conversation

poconn
Copy link
Contributor

@poconn poconn commented Oct 18, 2021

Adds iter_allocated_chunks_raw(), which (a) yields raw (*mut u8, usize), and (b) only borrows the arena immutably.

Fixes #121, follow-up from PR #126 (posting a new one as all of the previous changes would be reverted)

I need to finish the docs and add tests, posting just to get confirmation on this approach first.

Thanks!

Copy link
Owner

@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.

Yeah this looks good.

Can you add some smoke tests for iteration as well? Maybe a quickcheck that allocates a bunch of things, records the ranges where the things are allocated, and then makes sure that every allocated thing is within one of the chunks as returned by iter_allocated_chunks_raw? See tests/quickchecks.rs for the other quickchecks, which should give you an idea of how to write this kind of test.

Thanks!

src/lib.rs Outdated Show resolved Hide resolved
tests/quickchecks.rs Outdated Show resolved Hide resolved
@poconn
Copy link
Contributor Author

poconn commented Oct 19, 2021

Added smoke tests. No unit tests, but hopefully the existing iter_allocated_chunks() test cases along with the added chunks_and_raw_chunks_are_same() provide sufficient coverage? (especially considering that the old method now just wraps iter_allocated_chunks_raw())

@poconn poconn marked this pull request as ready for review October 19, 2021 00:43
Copy link
Owner

@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.

Tests look great!

Couple nitpicks below and then this should be ready to merge.

Thanks!

tests/quickchecks.rs Outdated Show resolved Hide resolved
tests/quickchecks.rs Outdated Show resolved Hide resolved
tests/quickchecks.rs Show resolved Hide resolved
Copy link
Owner

@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!

@fitzgen fitzgen merged commit fda6338 into fitzgen:master Oct 19, 2021
@poconn
Copy link
Contributor Author

poconn commented Oct 19, 2021

Thanks @fitzgen! If it's not too much trouble, we'd really appreciate if you could release a new version with the changes, please? :) (no hurry if you have other pending changes, just whenever is convenient)

@fitzgen
Copy link
Owner

fitzgen commented Oct 19, 2021

Published!

@poconn
Copy link
Contributor Author

poconn commented Oct 19, 2021

Great, thanks a lot!

@poconn poconn deleted the raw_chunk_iter branch October 19, 2021 18:12
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.

Allow iter_allocated_chunks on a &self
2 participants