Add more tests, overhaul CI, and fix plug soundness hole in bytemuck::offset_of!
#38
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I gave you a heads up in discord about this. Basically, I had a bunch of 80% finished work in
git stash
es, since I keep meaning to give bytemuck a bit more attention. Anyway I found some time and cleaned them up.... Which is to say, this PR is a bit of a hodge-podgey mess of things that are useful but not meaningfully related. It's not too big, but it's also not ideal to review.
That said, what's here is pretty nice maybe? In order of smallest to largest:
(first commit) Some minor doc fixing I noticed a while ago where you call types
T
andU
but the function takesA
andB
type params. IMO in an ideal world these would beFrom
andTo
but at a minimum the docs should use the right names.(third commit) Add some more unit tests. I ran a coverage scan of
bytemuck
at one point and it only has like 50%. Many of it's functions are very small and unlikely to hold bugs, but it doesn't really feel acceptable to have a crate with so much unsafe to have low coverage. Eventually I'll probably add more.(fourth commit) Totally rewrite the CI. I was concervative in what I forced on you here, but now it supports:
All the same stuff it was doing, e.g. test on 1.34.0, stable, beta, nightly on linux-x86_64.
Direct (non-
cross
) tests onmacOS-x86_64
,windows-{x86_64,i686}-{msvc,gnu}
.cross
-based tests onlinux-i686
, and 32-bit/64-bit mips (mips is obscure but is a big endian system which doesn't cause much trouble when run incross
cough unlike ppc cough)Running tests under
miri
,asan
,msan
, andlsan
.I didn't add anything that might be a little controversial, like lint/fmt/coverage/doc-check, but you can take a look at what I considered here: https://gist.github.com/thomcc/4d37f0ae675b3a2cba9ca5a7e2ba8baa. Of these IMO doc check is the most useful, since it makes it easier to be confident about cross-referencing in the docs.
(Oh and ofc I totally eyeballed this and can't test it locally so I'll probably have to add some followup commits to ensure it compiles. As you do).
(third commit) Fix the soundness hole in
bytemuck::offset_of!
, and overhaul it's docs. This is done using#[forbid(safe_packed_borrows)]
inside the macro body. This works at least back to the MSRV, didn't check any further.It comes with
compile_fail
tests (to ensure soundness), and an explanation that basically boils down to "No, this doesn't just mean you should wrap the macro inunsafe
".This is p. straightforward, but probably harder to review than the others since it's an API change that impacts safety.