From 4c4c43d338a261157d48ea55bfb4961eca1e3004 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sun, 7 Aug 2022 19:42:45 +0100 Subject: [PATCH 1/6] boom --- crates/bevy_ecs/src/world/mod.rs | 34 ++++++++------------------------ 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index d849d4a39914a..fb08555231a21 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -689,7 +689,9 @@ impl World { pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); // SAFETY: component_id just initialized and corresponds to resource of type T - unsafe { self.insert_resource_with_id(component_id, value) }; + OwningPtr::make(value, |ptr| unsafe { + self.insert_resource_by_id(component_id, ptr) + }); } /// Inserts a new non-send resource with standard starting values. @@ -717,7 +719,9 @@ impl World { self.validate_non_send_access::(); let component_id = self.components.init_non_send::(); // SAFETY: component_id just initialized and corresponds to resource of type R - unsafe { self.insert_resource_with_id(component_id, value) }; + OwningPtr::make(value, |ptr| unsafe { + self.insert_resource_by_id(component_id, ptr) + }); } /// Removes the resource of a given type and returns it, if it exists. Otherwise returns [None]. @@ -1233,24 +1237,6 @@ impl World { self.get_resource_unchecked_mut_with_id(component_id) } - /// # Safety - /// `component_id` must be valid and correspond to a resource component of type `R` - #[inline] - unsafe fn insert_resource_with_id(&mut self, component_id: ComponentId, value: R) { - let change_tick = self.change_tick(); - let column = self.initialize_resource_internal(component_id); - if column.is_empty() { - // SAFETY: column is of type R and has been allocated above - OwningPtr::make(value, |ptr| { - column.push(ptr, ComponentTicks::new(change_tick)); - }); - } else { - // SAFETY: column is of type R and has already been allocated - *column.get_data_unchecked_mut(0).deref_mut::() = value; - column.get_ticks_unchecked_mut(0).set_changed(change_tick); - } - } - /// Inserts a new resource with the given `value`. Will replace the value if it already existed. /// /// **You should prefer to use the typed API [`World::insert_resource`] where possible and only @@ -1258,6 +1244,7 @@ impl World { /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world + /// `component_id` must exist in this [`World`] pub unsafe fn insert_resource_by_id( &mut self, component_id: ComponentId, @@ -1265,12 +1252,7 @@ impl World { ) { let change_tick = self.change_tick(); - self.components().get_info(component_id).unwrap_or_else(|| { - panic!( - "insert_resource_by_id called with component id which doesn't exist in this world" - ) - }); - // SAFETY: component_id is valid, checked by the lines above + // SAFETY: component_id is valid, ensured by caller let column = self.initialize_resource_internal(component_id); if column.is_empty() { // SAFETY: column is of type R and has been allocated above From cdc5d162580f34ba17d07dd3633448d9f1eb3729 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sun, 7 Aug 2022 19:51:26 +0100 Subject: [PATCH 2/6] remove_test --- crates/bevy_ecs/src/world/mod.rs | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index fb08555231a21..4f1d48e40bd87 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1577,7 +1577,7 @@ mod tests { use super::World; use crate::{ change_detection::DetectChanges, - component::{ComponentDescriptor, ComponentId, ComponentInfo, StorageType}, + component::{ComponentDescriptor, ComponentInfo, StorageType}, ptr::OwningPtr, }; use bevy_ecs_macros::Component; @@ -1795,20 +1795,6 @@ mod tests { assert_eq!(DROP_COUNT.load(std::sync::atomic::Ordering::SeqCst), 1); } - #[test] - #[should_panic = "insert_resource_by_id called with component id which doesn't exist in this world"] - fn insert_resource_by_id_invalid_component_id() { - let invalid_component_id = ComponentId::new(usize::MAX); - - let mut world = World::new(); - OwningPtr::make((), |ptr| { - // SAFETY: ptr must be valid for the component_id `invalid_component_id` which is invalid, but checked by `insert_resource_by_id` - unsafe { - world.insert_resource_by_id(invalid_component_id, ptr); - } - }); - } - #[derive(Component)] struct Foo; From bb405327264dd523d6ad1835293de488cb450647 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sun, 7 Aug 2022 19:51:56 +0100 Subject: [PATCH 3/6] add `#[inline]` --- crates/bevy_ecs/src/world/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 4f1d48e40bd87..7d42ecf097262 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1245,6 +1245,7 @@ impl World { /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world /// `component_id` must exist in this [`World`] + #[inline] pub unsafe fn insert_resource_by_id( &mut self, component_id: ComponentId, From d4449a6941d360cf6f2a4cdeb44c00302851dc48 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sun, 7 Aug 2022 20:40:48 +0100 Subject: [PATCH 4/6] clippy rustfmt fight --- crates/bevy_ecs/src/world/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 7d42ecf097262..f65865021bad2 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -688,9 +688,11 @@ impl World { #[inline] pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); - // SAFETY: component_id just initialized and corresponds to resource of type T - OwningPtr::make(value, |ptr| unsafe { - self.insert_resource_by_id(component_id, ptr) + OwningPtr::make(value, |ptr| { + // SAFETY: component_id just initialized and corresponds to resource of type T + unsafe { + self.insert_resource_by_id(component_id, ptr); + } }); } @@ -718,9 +720,11 @@ impl World { pub fn insert_non_send_resource(&mut self, value: R) { self.validate_non_send_access::(); let component_id = self.components.init_non_send::(); - // SAFETY: component_id just initialized and corresponds to resource of type R - OwningPtr::make(value, |ptr| unsafe { - self.insert_resource_by_id(component_id, ptr) + OwningPtr::make(value, |ptr| { + // SAFETY: component_id just initialized and corresponds to resource of type R + unsafe { + self.insert_resource_by_id(component_id, ptr); + } }); } From c606e892bde9fa64dcdcfabe5cc9f636e2f608fd Mon Sep 17 00:00:00 2001 From: Boxy Date: Sun, 7 Aug 2022 22:07:48 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Jakob Hellermann --- crates/bevy_ecs/src/world/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f65865021bad2..ff1444ac8e5cf 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -689,7 +689,7 @@ impl World { pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); OwningPtr::make(value, |ptr| { - // SAFETY: component_id just initialized and corresponds to resource of type T + // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { self.insert_resource_by_id(component_id, ptr); } @@ -721,7 +721,7 @@ impl World { self.validate_non_send_access::(); let component_id = self.components.init_non_send::(); OwningPtr::make(value, |ptr| { - // SAFETY: component_id just initialized and corresponds to resource of type R + // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { self.insert_resource_by_id(component_id, ptr); } From c0fe2718d7b1e4dee2049698493f9adc6fb82d28 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 30 Aug 2022 13:22:15 -0700 Subject: [PATCH 6/6] remove unused function (might be needed in the future, but we can just re-add it ... it lives on in our history) --- crates/bevy_ecs/src/storage/table.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index d1c83e4d6bc45..bdfbf51e0f9a5 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -96,14 +96,6 @@ impl Column { self.data.is_empty() } - /// # Safety - /// index must be in-bounds - #[inline] - pub(crate) unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { - debug_assert!(row < self.len()); - self.ticks.get_unchecked_mut(row).get_mut() - } - /// # Safety /// index must be in-bounds #[inline]