-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
BTree: evaluate static type-related check at compile time #95005
Conversation
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
impl BorrowType for Owned { | ||
// Traversal isn't needed, it happens using the result of `borrow_mut`. | ||
// Reject evaluation, because traversal isn't needed. Instead traversal | ||
// happens using the result of `borrow_mut`. | ||
// By disabling traversal, and only creating new references to roots, | ||
// we know that every reference of the `Owned` type is to a root node. | ||
const PERMITS_TRAVERSAL: bool = false; | ||
const TRAVERSAL_PERMIT: () = panic!(); | ||
} |
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.
Can you add some UI tests to show the cases where this does and doesn't panic and to let us review the panic output?
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.
I don't think I can. These static asserts guard the private internals of node.rs. We have unit test of those internals, we can do positive compile tests, but a negative compile test of internals, I doubt the test framework caters for that. And surely we don't want to make this stuff public.
The way I tested this is to try to address the remaining FIXME on reborrow_mut
in node.rs: disable the permit for Mut<'+>
like it is disabled for Owned
, and build errors point out the first place where a Mut
handle is traversed. While without this PR, you have to run tests to see it, and you have to assume tests offer complete coverage. I've edited the PR description a bit.
r? rust-lang/libs |
r? @thomcc |
This looks fine to me. Sorry for the delay. @bors r+ rollup |
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#95005 (BTree: evaluate static type-related check at compile time) - rust-lang#99742 (Add comments about stdout locking) - rust-lang#100128 (Document that `RawWakerVTable` functions must be thread-safe.) - rust-lang#100956 (Reduce right-side DOM size) - rust-lang#101006 (Fix doc cfg on reexports) - rust-lang#101012 (rustdoc: remove unused CSS for `.variants_table`) - rust-lang#101023 (rustdoc: remove `type="text/css"` from stylesheet links) - rust-lang#101031 (Remove unused build dependency) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
assert
s like the ones replaced here would only go off when you run the right test cases, if the code were ever incorrectly changed such that rhey would trigger. But inspired on a nice forum question, they can be checked at compile time.