search

rustwasm / wee_alloc

The Wasm-Enabled, Elfin Allocator
Mozilla Public License 2.0
665 stars 49 forks source link

Fix free_cell_layout test #70

Closed RReverser closed 5 years ago

RReverser commented 5 years ago

Fixes #69.

ZackPierce commented 5 years ago

I wonder if we could also implement some of these sorts of sizing checks as compile-time assertions without affecting binary size, and thus move some key sanity assumptions earlier in the QA cycle (that is, theoretically get caught on cargo check rather than just cargo test).

RReverser commented 5 years ago

@ZackPierce That should be definitely possible as all involved checks are just using const funcs.

fitzgen commented 5 years ago

I wonder if we could also implement some of these sorts of sizing checks as compile-time assertions without affecting binary size, and thus move some key sanity assumptions earlier in the QA cycle (that is, theoretically get caught on cargo check rather than just cargo test).

It would be nicer to get the errors as early as possible, but the code size isn't really a concern, since the tests are only compiled when running cargo test and are not compiled for cargo build.

ZackPierce commented 5 years ago

If the appropriate static assertions could be placed in the regular compile path (that is, not in a compile-for-test-only module) and had no effects on code size or runtime, then it seems like a good idea to get that extra level of compiler-help for development applied as broadly as reasonable.

fitzgen commented 5 years ago

Rust doesn't have static assertions though. You could have #[test]s with const fn but I don't think that would catch anything earlier, Rust would have to also add #[const_test] or something. Am I missing something?

RReverser commented 5 years ago

@fitzgen Yeah but there are plenty of crates that internally e.g. convert boolean to 0 or -1 and then use that as size of array type, or just do integer overflow and so on, basically forcing a compilation error.

RReverser commented 5 years ago

But yeah, for now cargo test seems enough IMO.

fitzgen commented 5 years ago

@fitzgen Yeah but there are plenty of crates that internally e.g. convert boolean to 0 or -1 and then use that as size of array type, or just do integer overflow and so on, basically forcing a compilation error.

Oh, wow! That is a clever trick! Would be happy to merge a PR that added this :-p

ZackPierce commented 5 years ago

Agreed, tests (and this timely PR) seem good enough for now; was looking for ways to reduce the odds of such a mismatch escaping us in the future. For reference, one might use https://github.com/nvzqz/static-assertions-rs or similar.

RReverser commented 5 years ago

@ZackPierce Yeah, that was one of the crates I was referring to. They implement static assertions using 0 or -1 approach: https://github.com/nvzqz/static-assertions-rs/blob/master/src/const_assert.rs#L74-L75

RReverser commented 5 years ago

was looking for ways to reduce the odds of such a mismatch escaping us in the future

Well for that tests just have to be executed at all (in particular, on CI). I've raised #72 to track that.