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

Replace each_allocated_chunk function with iter_allocated_chunks function #30

Merged
merged 7 commits into from
Oct 29, 2019
Merged

Replace each_allocated_chunk function with iter_allocated_chunks function #30

merged 7 commits into from
Oct 29, 2019

Conversation

TethysSvensson
Copy link
Contributor

This implements the suggestion in #27.

@TethysSvensson
Copy link
Contributor Author

Note that this currently removes the function each_allocated_chunk, since I believe this new functionality to be strictly superior. The iterator API is more usable, while also being safe to use as long as you do not look at any of the byte values. The type signature makes it clear exactly which parts are unsafe.

I can easily re-implement the old function on top of the iterator for backwards-compatiblity, if you prefer. If you want me to do this, I think we should consider marking as deprecated, or perhaps changed to use [MaybeUninit<u8>] slices instead of [u8] slices (though that you also be a breaking change).

@TethysSvensson
Copy link
Contributor Author

By using google and github search, I have not been able to find any references to each_allocated_chunk outside of this crate or rustdocs. There might be users of the function in private or non-indexed code though.

@fitzgen
Copy link
Owner

fitzgen commented Oct 28, 2019

By using google and github search, I have not been able to find any references to each_allocated_chunk outside of this crate or rustdocs. There might be users of the function in private or non-indexed code though.

I use it here, fwiw: https://github.com/fitzgen/dodrio/blob/22faf1eb60f9bcb16094e1b2f4575edfb203338e/src/change_list/emitter.rs#L43

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.

Looks great, thanks @TethysSvensson!

Can you also reimplement each_allocated_chunk in terms of iter_allocated_chunks with a #[deprecated(note = "deprecated in favor of iter_allocated_chunks")] note?

Thanks!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
TethysSvensson and others added 6 commits October 29, 2019 09:28
Co-Authored-By: Nick Fitzgerald <[email protected]>
Co-Authored-By: Nick Fitzgerald <[email protected]>
Co-Authored-By: Nick Fitzgerald <[email protected]>
Co-Authored-By: Nick Fitzgerald <[email protected]>
Co-Authored-By: Nick Fitzgerald <[email protected]>
@TethysSvensson
Copy link
Contributor Author

By using google and github search, I have not been able to find any references to each_allocated_chunk outside of this crate or rustdocs. There might be users of the function in private or non-indexed code though.

I use it here, fwiw: https://github.com/fitzgen/dodrio/blob/22faf1eb60f9bcb16094e1b2f4575edfb203338e/src/change_list/emitter.rs#L43

That also shows up on github search now, but didn't before. It still does not show up on google search. This makes me a lot less inclined to trust github search in the future.

Can you also reimplement each_allocated_chunk in terms of iter_allocated_chunks with a #[deprecated(note = "deprecated in favor of iter_allocated_chunks")] note?

Done.

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 so much!

@fitzgen
Copy link
Owner

fitzgen commented Oct 29, 2019

That also shows up on github search now, but didn't before. It still does not show up on google search. This makes me a lot less inclined to trust github search in the future.

FWIW, I think github will only show the first two hits in a file, so if there are multiple occurrences, they might be hidden. In the specific dodrio example, the first two occurrences are inside comments, so the actual call is hidden :-/

@fitzgen fitzgen merged commit 1fec3b8 into fitzgen:master Oct 29, 2019
@TethysSvensson TethysSvensson deleted the iterator branch October 29, 2019 17:50
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