-
Notifications
You must be signed in to change notification settings - Fork 10
Maintain range information on get for optimisation purposes #29
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
Maintain range information on get for optimisation purposes #29
Conversation
|
This seems like a good change. But is there any way we can add a test for this, something that can run in CI? Maybe with iai, or with cargo asm or something? Unfortunately I don’t know of a tool that makes this sort of thing easy…
|
|
Btw, I fixed the CI failures, so you can rebase on/merge with main. |
|
Nevermind, someone pointed out to me a better solution. Make another library crate in the workspace and put in its [lib]
crate-type = ["cdylib"]Add this to its unsafe extern "C" {
safe fn should_be_optimized_out() -> !;
}
fn assert(b: bool) {
if !b {
should_be_optimized_out();
}
}In CI, run the command #[unsafe(no_mangle)]
pub fn simple_range(i: &BoundedU8<3, 5>) {
assert(3 <= i.get());
assert(i.get() <= 5);
} |
|
Also worth noting that I think this statement is only necessary for the struct definitions, as the |
|
I like the idea! I ran the automated tests under Miri, which is usually pretty good at finding undef. Great idea to use linker failures to handle this though! EDIT: I also quickly manually tested that the asserts are optimised out using |
fc453b0 to
7364647
Compare
…1.90.0, even though it seems like it should
…ilds, add `miri test` for additional checks for undef
|
Ok, I added tests to ensure that the range checks are optimized out as expected. |
| "uses": "actions-rs/cargo@v1", | ||
| "with": { | ||
| "command": "miri", | ||
| # `miri` does not support the `macros` feature as it uses IO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, really? cargo +nightly miri test --workspace --all-features works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! I’ll investigate tomorrow
|
Made some changes based on your review. The name I went with in the end was |
|
@Kestrer Hey, any movement on this? |
|
Sorry for the delay. I didn’t forget but I’ve been rather sick… |
Small PR to ensure that the range information is persisted when getting the value. This allows
bounded-integerto be used not just for correctness, but for optimisation too.