Simplify unaligned Ref constructors #329
Conversation
joshlf
force-pushed
the
known-layout-1-simplify-unaligned-ref
branch
2 times, most recently
from
971c23f to
673a809
Compare
src/lib.rs
Outdated
| #[inline] | ||
| pub fn new_unaligned(bytes: B) -> Option<Ref<B, T>> { | ||
| if bytes.len() != mem::size_of::<T>() { | ||
| return None; | ||
| } | ||
| Some(Ref(bytes, PhantomData)) | ||
| Ref::new(bytes) | ||
| } |
There was a problem hiding this comment.
Since these methods just become aliases for their non-unaligned counterparts, what's the rationale for keeping them? If it's just to minimize immediate breakage, I'd suggest that they are mark deprecated and removed from a future release.
There was a problem hiding this comment.
It's to minimize breakage, yeah. I don't want to mark them as deprecated yet for two reasons:
- Without deprecating them, this is a breaking change [1]
- I'm not convinced we'll actually want to get rid of them; there's more work to be done around how our API expresses what is and isn't validated at runtime, and I could see a future in which we keep these and have them return a different error type than the constructors that need to check alignment
I've filed #338 to track this.
[1] At some point I want to come up with a more unified story around how we keep track of "pending breaking changes". It'd be good to be able to say "this is a breaking change we'll make as soon as we're ready to release e.g. 0.8." We have that loosely tracked in issues right now, but it's neither consistent nor enforced.
These constructors don't currently make use of the `T: Unaligned` bound internally - they just use it to document that an alignment check is guaranteed to be elided. The alignment check in the non-`Unaligned` constructors is trivially optimizable when it's unneeded since it's just a check that `x % 1 == 0`. For that reason, in this commit, we remove these methods' internals and simply have them be wrappers around the equivalent non-`Unaligned` constructors. While we're here, mark `Ref` functions which are just thin wrappers around other functions as `#[inline(always)]`.
joshlf
force-pushed
the
known-layout-1-simplify-unaligned-ref
branch
from
673a809 to
d97190e
Compare
|
Looks like pushing new changes removed your approval, sorry. Can you re-approve? |
These constructors don't currently make use of the
T: Unalignedbound internally - they just use it to document that an alignment check is guaranteed to be elided. The alignment check in the non-Unalignedconstructors is trivially optimizable when it's unneeded since it's just a check thatx % 1 == 0. For that reason, in this commit, we remove these methods' internals and simply have them be wrappers around the equivalent non-Unalignedconstructors.