-
Notifications
You must be signed in to change notification settings - Fork 221
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
Safety #765
Safety #765
Changes from 40 commits
2f331ad
8366ddb
ba8b976
ac2146d
bf620e1
e7d0aa1
cc97588
483914b
57e929c
0968089
25b27e5
448c9de
919434b
d2d05f2
ce90ce7
bea1ea9
e22d22a
738dc16
e4c3655
6ca2338
0fad41d
2c7d5e6
c33cd7c
09f1a4c
c3a301f
e8f80e7
391b310
6b74393
5aa5abf
2beb089
cbfa283
819d6ca
2400172
6736f15
a31e781
1fc3d1f
5abd46a
de30c85
35da879
a8c96f8
784c7b0
5d3bfe7
a53d28e
0a4b99a
6d4724b
eecf83a
276450e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[profile.default-miri] | ||
slow-timeout = { period = "30s", terminate-after = 1 } | ||
fail-fast = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,8 @@ | ||
hard_tabs = false | ||
imports_granularity = "Crate" | ||
reorder_impl_items = true | ||
use_field_init_shorthand = true | ||
use_try_shorthand = true | ||
format_code_in_doc_comments = true | ||
wrap_comments = true | ||
edition = "2018" | ||
edition = "2021" | ||
version = "Two" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,8 +117,8 @@ fetch the component as mutable if/when needed. | |
for (entity, mut comp) in (&entities, &mut comps.restrict_mut()).join() { | ||
// Check whether this component should be modified, without fetching it as | ||
// mutable. | ||
if comp.get_unchecked().condition < 5 { | ||
let mut comp = comp.get_mut_unchecked(); | ||
if comp.get().condition < 5 { | ||
let mut comp = comp.get_mut(); | ||
Comment on lines
+120
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The _unchecked suffix always reminds me of unsafe access (e.g. https://doc.rust-lang.org/std/primitive.slice.html#method.get_unchecked ), so this rename is good. Update: I later realized that get indeed remains unsafe, and the restriction that the item must exists sounds very similar to the unchecked variant of slice. 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The method discussed here has always been safe. There are other unsafe methods with the same name that this PR touches, so maybe you are thinking of one of those. |
||
// ... | ||
} | ||
} | ||
|
@@ -137,4 +137,4 @@ simply call `storage.set_event_emission(true)`. | |
|
||
_See | ||
[FlaggedStorage Doc](https://docs.rs/specs/latest/specs/struct.FlaggedStorage.html) | ||
for more into._ | ||
for more into._ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
use specs::prelude::*; | ||
struct Pos(f32); | ||
|
||
impl Component for Pos { | ||
type Storage = VecStorage<Self>; | ||
} | ||
|
||
fn main() { | ||
let mut world = World::new(); | ||
|
||
world.register::<Pos>(); | ||
|
||
let entity0 = world.create_entity().with(Pos(0.0)).build(); | ||
world.create_entity().with(Pos(1.6)).build(); | ||
world.create_entity().with(Pos(5.4)).build(); | ||
|
||
let mut pos = world.write_storage::<Pos>(); | ||
let entities = world.entities(); | ||
|
||
// Unlike `join` the type return from `lend_join` does not implement | ||
// `Iterator`. Instead, a `next` method is provided that only allows one | ||
// element to be accessed at once. | ||
let mut lending = (&mut pos).lend_join(); | ||
|
||
// We copy the value out here so the borrow of `lending` is released. | ||
let a = lending.next().unwrap().0; | ||
// Here we keep the reference from `lending.next()` alive, so `lending` | ||
// remains exclusively borrowed for the lifetime of `b`. | ||
let b = lending.next().unwrap(); | ||
// This right fails to compile since `b` is used below: | ||
// let d = lending.next().unwrap(); | ||
b.0 = a; | ||
|
||
// Items can be iterated with `while let` loop: | ||
let mut lending = (&mut pos).lend_join(); | ||
while let Some(pos) = lending.next() { | ||
pos.0 *= 1.5; | ||
} | ||
|
||
// A `for_each` method is also available: | ||
(&mut pos).lend_join().for_each(|pos| { | ||
pos.0 += 1.0; | ||
}); | ||
|
||
// Finally, there is one bonus feature which `.join()` can't soundly provide. | ||
let mut lending = (&mut pos).lend_join(); | ||
// That is, there is a method to get the joined result for a particular | ||
// entity: | ||
if let Some(pos) = lending.get(entity0, &entities) { | ||
pos.0 += 5.0; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
#!/bin/bash | ||
# | ||
# Convenience script for running Miri, also the same one that the CI runs! | ||
|
||
# use half the available threads since miri can be a bit memory hungry | ||
test_threads=$((($(nproc) - 1) / 2 + 1)) | ||
echo using $test_threads threads | ||
|
||
# filters out long running tests | ||
filter='not (test(100k) | test(map_test::wrap) | test(map_test::insert_same_key) | test(=mixed_create_merge)| test(=par_join_many_entities_and_systems) | test(=stillborn_entities))' | ||
echo "using filter: \"$filter\"" | ||
|
||
# Miri currently reports leaks in some tests so we disable that check | ||
# here (might be due to ptr-int-ptr in crossbeam-epoch so might be | ||
# resolved in future versions of that crate). | ||
MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-ignore-leaks" \ | ||
cargo +nightly miri nextest run \ | ||
-E "$filter" \ | ||
--test-threads="$test_threads" \ | ||
# use nocapture or run miri directly to see warnings from miri | ||
#--nocapture |
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.
CI now includes running miri.