Fix stacked-borrows violations in Vector #83
Conversation
|
Depends on this: jneem/imbl-sized-chunks#8 |
| @@ -936,6 +971,26 @@ where | |||
| Some(&mut self.get_focus()[target_phys_index]) | |||
| } | |||
|
|
|||
| pub fn get_many<const N: usize>( | |||
There was a problem hiding this comment.
Maybe call this get_many_mut to match the other one? Also, does it need to be pub? If so, I think it needs to be documented. If not, maybe you could even make all the internal functions unsafe and skip the index-checking. Because internally we're already checking indices in, e.g. triplet
There was a problem hiding this comment.
I skipped the mut because the above get method also skips it, despite returning a mutable reference. In terms of needing to be pub, it doesn't really need to be, though it may be useful to users. To match the std pattern of such APIs, if it stays public, there should likely be an unsafe variant that skips the checks, then triplet can use it.
There was a problem hiding this comment.
Makes sense, thanks. I don't have a strong opinion about whether it should be pub or not, just that if it's pub then its documentation should be up to par with the documentation on other pub methods.
| @@ -246,26 +246,31 @@ mod test { | |||
| use serde_json::{from_str, to_string}; | |||
|
|
|||
| proptest! { | |||
| #[cfg_attr(miri, ignore)] | |||
There was a problem hiding this comment.
Could you explain in a comment somewhere the criterion for excluding a test from miri? Also, if you want to exclude a whole mod then maybe just #[cfg(not(miri))] at the top is easier instead of annotating every test.
There was a problem hiding this comment.
The logic behind not using cfg(not(miri)) is that it causes a bunch of 'unused import' warnings, and with ignore it's possible to use --ignored to run it anyways, if you really want to try waiting for it to complete.
I'll add some documentation when I get a minute. For posterity, it's basically just 'takes more than ~30 seconds to run', and some tests may actually prefer running but with reduced iteration values. I just didn't want to take the time to track down why the specific iteration numbers were chosen for many of the issue tests, to find out whether they could be reduced and still have the test be meaningful.
Also adds annotations to make running miri test easier for vector. This doesn't yet add those annotations to other places in the codebase. The
full_retaintest is needed to ensure tests coverTreeFocusMut::get_many.With these changes, miri reports no UB running the non-ignored parts of the test suite.