get rid of unnecessary uses of unsafe #460
Conversation
It turns out all of our uses of unsafe in this module can be trivally replaced by uses of the zerocopy crate. Signed-off-by: Tom Dohrmann <[email protected]>
Once again, zerocopy can do everything we needed without us using unsafe. Signed-off-by: Tom Dohrmann <[email protected]>
The new implementation is much shorter, should perform exactly the same and doesn't even rely on unsafe. Signed-off-by: Tom Dohrmann <[email protected]>
zerocopy is great :) Signed-off-by: Tom Dohrmann <[email protected]>
We can make use of the `Deref` and `DerefMut` implementations. Signed-off-by: Tom Dohrmann <[email protected]>
| // SAFETY: this is part of the invariants of this type, as it must | ||
| // hold a pointer to valid memory for the given `T`. | ||
| unsafe { self.ptr.as_ref() } | ||
| self |
There was a problem hiding this comment.
If this function doesn't require any manipulation of self before returning, then why keep the function body at all? I just tried an experiment where I completely removed the four functions you're touching here, and the compiler is perfectly happy, so it doesn't seem like we get any benefit from keeping functions that contain no logic.
There was a problem hiding this comment.
The reason is that these functions implement traits that make it easier to work with generics. For example any function that takes a generic type S such that S: AsRef<T> will also accept PageBox<T>.
Just because the code is not needed now doesn't mean we should remove it, like the usual derives we have on most types.
There was a problem hiding this comment.
Makes sense - thanks! I think that means that as we introduce other similar types (such as the introduction of SharedBox<T> in #451) then those types that can implicitly support AsRef<T> or borrow and the like should also implement similar skeletal functions so that they are there if/when needed.
There was a problem hiding this comment.
I haven't looked into that PR in depth, but I'd say that's a good idea, yes.
There was a problem hiding this comment.
SharedBox<T> doesn't allow creating references (shared or mutable) to the pointed-to data, so it doesn't need implementations for these traits.
This PR get's rid of some unnecessary uses of unsafe mainly by switching to safe alternatives provided by the zerocopy crate.