[unsafe-fields] Add as_ref_checked #1960
Conversation
joshlf
force-pushed
the
I7ce0c981ed1f1bc1f4ff85dffef2a74114c6e76d
branch
from
9092745
to
4289ead
Compare
unsafe-fields/src/lib.rs
Outdated
| #[inline(always)] | ||
| #[cfg(feature = "zerocopy_0_8")] | ||
| #[cfg_attr(doc_cfg, doc(cfg(feature = "zerocopy_0_8")))] | ||
| pub const fn as_ref_checked(&self) -> &F |
There was a problem hiding this comment.
My preference would be as_ref_unchecked for the unsafe method, and make the safe thing the shortest to type (.as_ref). Most types are Immutable, so I additionally expect the safe version will be the common case!
There was a problem hiding this comment.
Feels weird to have most of the API suffixed with _unchecked, but I agree that that's philosophically the right thing to do.
Done.
joshlf
force-pushed
the
I7ce0c981ed1f1bc1f4ff85dffef2a74114c6e76d
branch
3 times, most recently
from
12189bf
to
f1391d2
Compare
There was a problem hiding this comment.
I don't think we necessarily need to name everything _unchecked. The _unchecked prefix in Rust is conventionally used in Rust when there's a safe option that the method needs to be distinguished from.
When there's no safe alternative, the standard library omits the _unchecked suffix (e.g., contrast get and get_unchecked with slice::align_to).
unsafe-fields/src/lib.rs
Outdated
| @@ -253,7 +277,7 @@ impl<O: ?Sized, F, const NAME_HASH: u128> Unsafe<O, F, { NAME_HASH }> { | |||
| /// The caller is responsible for upholding any safety invariants associated | |||
| /// with this field. | |||
| #[inline(always)] | |||
| pub const unsafe fn into(self) -> F { | |||
| pub const unsafe fn into_unchecked(self) -> F { | |||
There was a problem hiding this comment.
This should be safe, right? It consumes self, so F cannot possibly have any safety invariants with respect to self.
unsafe-fields/src/lib.rs
Outdated
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller is responsible for upholding any safety invariants associated | ||
| /// with this field. | ||
| #[inline(always)] | ||
| pub const unsafe fn new(field: F) -> Unsafe<O, F, { NAME_HASH }> { | ||
| pub const unsafe fn new_unchecked(field: F) -> Unsafe<O, F, { NAME_HASH }> { |
There was a problem hiding this comment.
I'm fine just calling this new, since we couldn't possibly offer a safe variant that we'd need to distinguish it from. All cases of initialization are unsafe, without exception.
unsafe-fields/src/lib.rs
Outdated
| /// Gets a mutable reference to the inner value. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller is responsible for upholding any safety invariants associated | ||
| /// with this field. | ||
| #[inline(always)] | ||
| pub unsafe fn as_mut(&mut self) -> &mut F { | ||
| pub unsafe fn as_mut_unchecked(&mut self) -> &mut F { |
There was a problem hiding this comment.
Same concern as new. I don't think there's a safe version we could possibly offer here that we'd want to distinguish the unsafe version from.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
=======================================
Coverage 89.46% 89.46%
=======================================
Files 16 16
Lines 5838 5838
=======================================
Hits 5223 5223
Misses 615 615 ☔ View full report in Codecov by Sentry. |
joshlf
force-pushed
the
I7ce0c981ed1f1bc1f4ff85dffef2a74114c6e76d
branch
3 times, most recently
from
7835610
to
d459581
Compare
Release 0.2.0. Makes progress on #1931 gherrit-pr-id: I7ce0c981ed1f1bc1f4ff85dffef2a74114c6e76d
joshlf
force-pushed
the
I7ce0c981ed1f1bc1f4ff85dffef2a74114c6e76d
branch
from
d459581
to
1f83c62
Compare
Release 0.2.0.
Makes progress on #1931
This PR is on branch unsafe-fields-zerocopy.