Skip to content

Conversation

@ninehusky
Copy link
Contributor

@ninehusky ninehusky commented Nov 17, 2025

Adds no_panic specs for the following functions:

core::slice::<impl [T]>::len: 9
core::num::<impl usize>::saturating_sub: 1
core::slice::<impl [T]>::split_at: 1
core::slice::index::<impl core::ops::Index<I> for [T]>::index: 2
core::ops::Fn::call: 1
core::ops::FnMut::call_mut: 1

@ninehusky ninehusky changed the title Add extern specs for all ring buffer functions Add extern specs for all core functions used by ring buffer Nov 17, 2025
Copy link
Collaborator

@enjhnsn2 enjhnsn2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. Specs appear to be sound, but are currently trusted, but it would be good to at least attempt to verify them at some point.

v[0]
}

#[cfg_attr(flux, flux::no_panic)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not just #[flux::no_panic]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nilehmann deferring to you for a smarter explanation. From what I remember, this has to do with Rust's mechanism for attaching attributes to modules? There's something about #[no_panic] becoming a "macro within a macro" which causes issues when showing up as a module annotation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a test, this is actually not needed, because we never run rustc directly on tests. When using this for real, you can't write flux::no_panic directly because during normal compilation, rustc will complain about flux::no_panic not existing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, but you can do flux_rs::no_panic, correct? Isn't flux_rs the standard way to invoke preconditions/postconditions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. For this particular example, flux_rs::no_panic would work, but it won't work for file module declaration (the common version of declaring modules), i.e., the following doesn't work.

#[flux_rs::no_panic]
mod my_module;

@@ -0,0 +1,22 @@
#[cfg_attr(flux, flux::no_panic)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echo above comment about this being #[flux::no_panic]

}


#[cfg_attr(flux, flux::no_panic)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[flux::no_panic]

@ninehusky
Copy link
Contributor Author

@enjhnsn2 @nilehmann are we good to punt the conversation re: cfg_attr(flux, no_panic) to a different PR? if so, this one is ready for review.

@nilehmann
Copy link
Member

@enjhnsn2 @nilehmann are we good to punt the conversation re: cfg_attr(flux, no_panic) to a different PR? if so, this one is ready for review.

That part is already merged so I don't know why we were discussing it at all here 😅

@ninehusky ninehusky marked this pull request as ready for review November 20, 2025 17:57
@ninehusky
Copy link
Contributor Author

OK, marking this ready for review then. I think the gh-pages step in CI is failing on this branch, but the last PR may have fixed that?

@nilehmann
Copy link
Member

@ninehusky LGTM. Can you rebase and push to verify CI is green after #1382

@ninehusky
Copy link
Contributor Author

@nilehmann done!

@nilehmann nilehmann merged commit ecc2445 into flux-rs:main Nov 20, 2025
7 checks passed
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.

3 participants