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 SplitByteSlice::split_at_unchecked #1071

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Add SplitByteSlice::split_at_unchecked #1071

merged 2 commits into from
Apr 18, 2024

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Mar 25, 2024

Given the importance of avoiding panic paths to some of our customers, we should, as a rule, avoid only offering (or relying upon) APIs with panic paths.

Related to #202

@jswrenn jswrenn force-pushed the try_split branch 2 times, most recently from 748ec27 to 4bac39f Compare March 27, 2024 19:32
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
Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

I worry that this commits us to an API - try_split_at - that we're not 100% sure we want long-term. The only use of it in this PR is internal to the crate, so maybe we could just make it a private bare function? We'd keep the introduction of split_at_unchecked, and we'd keep split_at as a method with a default impl. Those changes need to go in now (before 0.8) since they're breaking changes. But making try_split_at a private function accomplishes the same goals as this PR but commits us to fewer APIs long-term.

@joshlf joshlf mentioned this pull request Mar 27, 2024
4 tasks
@jswrenn jswrenn changed the title Add SplitByteSlice::{try_split_at, split_at_unchecked} Add SplitByteSlice::split_at_unchecked Mar 28, 2024
@jswrenn
Copy link
Collaborator Author

jswrenn commented Mar 28, 2024

The only use of it in this PR is internal to the crate, so maybe we could just make it a private bare function?

Done.

@jswrenn jswrenn requested a review from joshlf March 28, 2024 19:21
src/lib.rs Show resolved Hide resolved
src/util.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
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
@jswrenn jswrenn force-pushed the try_split branch 2 times, most recently from ade377a to b2e4740 Compare April 10, 2024 20:22
src/lib.rs Show resolved Hide resolved
src/util.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
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
@jswrenn jswrenn requested a review from joshlf April 15, 2024 14:57
src/lib.rs Outdated
Comment on lines 5798 to 5803
// `l_ptr` is non-null, because `self` is non-null, by invariant on
// `&mut [u8]`.
let l_ptr = self.as_mut_ptr();

// `l_ptr` is non-null, because `self` is non-null, by invariant on
// `&mut [u8]`.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe only say this (in comments) once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Given the importance of avoiding panic paths to some of our
customers, we should, as a rule, avoid only offering (or relying
upon) APIs with panic paths.

Related to #202
@joshlf
Copy link
Member

joshlf commented Apr 15, 2024

The Kani breakage is tracked upstream: model-checking/kani#3138

@jswrenn jswrenn added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit a42d1aa Apr 18, 2024
210 checks passed
@jswrenn jswrenn deleted the try_split branch April 18, 2024 03:04
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