Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more tests, overhaul CI, and fix plug soundness hole in bytemuck::offset_of! #38

Merged
merged 6 commits into from
Sep 7, 2020

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Sep 6, 2020

I gave you a heads up in discord about this. Basically, I had a bunch of 80% finished work in git stashes, 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:

  1. (first commit) Some minor doc fixing I noticed a while ago where you call types T and U but the function takes A and B type params. IMO in an ideal world these would be From and To but at a minimum the docs should use the right names.

  2. (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.

  3. (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 on macOS-x86_64, windows-{x86_64,i686}-{msvc,gnu}.

    • cross-based tests on linux-i686, and 32-bit/64-bit mips (mips is obscure but is a big endian system which doesn't cause much trouble when run in cross cough unlike ppc cough)

    • Running tests under miri, asan, msan, and lsan.

    • 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).

  4. (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 in unsafe".

    • This is p. straightforward, but probably harder to review than the others since it's an API change that impacts safety.

@Lokathor Lokathor merged commit 64cc597 into Lokathor:main Sep 7, 2020
@Lokathor
Copy link
Owner

Lokathor commented Sep 7, 2020

So this is just a patch release it seems like, no new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants