Remove _manual_slice constructors
#5312
Conversation
robertbastian
requested review from
zbraniecki,
sffc,
Manishearth,
nciric and
a team
as code owners
There was a problem hiding this comment.
We could have done this with unsafe originally, but I think we did it with _manual_slice in order to avoid unsafe. I'll defer to @Manishearth
In other words, I agree with the motivation for this change, but if it involves adding unsafe, I'm not convinced that the tradeoff is worth it
| pub const fn split_out_range(slice: &[u8], start: usize, end: usize) -> &[u8] { | ||
| assert!(start <= slice.len()); | ||
| assert!(end <= slice.len()); | ||
| assert!(start <= end); | ||
| // SAFETY: assertions and align = size = 1. | ||
| unsafe { core::slice::from_raw_parts(slice.as_ptr().add(start), end - start) } | ||
| } |
There was a problem hiding this comment.
split_at_checked is new (and const) in 1.80: rust-lang/rust#85122
I think I prefer holding this until we can use that in MSRV so we don't introduce unsafe code.
There was a problem hiding this comment.
We did this because clients didn't have a safe way to do this at all. I'm assuming the majority of our clients will be on stable, so now the majority of our clients have a safe way to do this. One line more or less unsafe code to make this work isn't the end of the world. Also note that I have a follow-up PR that simplifies the unsafety, but it needs to be benched.
There was a problem hiding this comment.
ok, if this is just about getting rid of _manual_slice in public APIs, then just remove those functions without adding unsafe code?
There was a problem hiding this comment.
The parser calls the _manual_slice functions, because it needs to const-slice the input. I'm doing this in one location in the parser, and this is not the only unsafe code in this crate.
There was a problem hiding this comment.
Please add at least a comment here saying something like
// TODO(MSRV): Use slice_at_checked in Rust 1.80
There was a problem hiding this comment.
I'm fine with doing this if we commit to using the safe API later. It's unsafe code being added to a crate (unfortunate), but it's rather straightforward, and const, and only one crate, so I'm not too bothered.
I would like us to move to split_at_checked when we can, though.
(Do we have an issue to keep track of all the rolling "let's fix this with the next MSRV" changes we keep deciding on? I think I saw one before but I don't know if we still have one)
There was a problem hiding this comment.
OK since @Manishearth approved, but let's record somewhere that we can target this unsafe block after MSRV moves
| pub const fn split_out_range(slice: &[u8], start: usize, end: usize) -> &[u8] { | ||
| assert!(start <= slice.len()); | ||
| assert!(end <= slice.len()); | ||
| assert!(start <= end); | ||
| // SAFETY: assertions and align = size = 1. | ||
| unsafe { core::slice::from_raw_parts(slice.as_ptr().add(start), end - start) } | ||
| } |
There was a problem hiding this comment.
Please add at least a comment here saying something like
// TODO(MSRV): Use slice_at_checked in Rust 1.80
We added these because range slicing
&foo[x..y]is not available in const. However, it can be worked around withslice::split_at/slice::split_at_checked/slice::split_at_unchecked. Removing this simplifies our API surface as well as the implementation.