diff --git a/crates/fj-kernel/src/insert.rs b/crates/fj-kernel/src/insert.rs index 28b5aed58..f1c104b3b 100644 --- a/crates/fj-kernel/src/insert.rs +++ b/crates/fj-kernel/src/insert.rs @@ -28,7 +28,9 @@ macro_rules! impl_insert { self, objects: &Objects, ) -> Result, ::Error> { - objects.$store.insert(self) + let handle = objects.$store.reserve(); + objects.$store.insert(handle.clone(), self)?; + Ok(handle) } } )* diff --git a/crates/fj-kernel/src/lib.rs b/crates/fj-kernel/src/lib.rs index 9eebf0122..6aaac3314 100644 --- a/crates/fj-kernel/src/lib.rs +++ b/crates/fj-kernel/src/lib.rs @@ -86,6 +86,11 @@ //! [Fornjot]: https://www.fornjot.app/ #![warn(missing_docs)] +// I've made a simple change that put `ValidationError` over the threshold for +// this warning. I couldn't come up with an easy fix, and figured that silencing +// the warning is the most practical solution for now, as the validation +// infrastructure is in flux anyway. Maybe the problem will take care of itself. +#![allow(clippy::result_large_err)] pub mod algorithms; pub mod builder; diff --git a/crates/fj-kernel/src/objects/stores.rs b/crates/fj-kernel/src/objects/stores.rs index 153e4a4fc..079e6d2b6 100644 --- a/crates/fj-kernel/src/objects/stores.rs +++ b/crates/fj-kernel/src/objects/stores.rs @@ -80,10 +80,20 @@ pub struct Curves { } impl Curves { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Curve`] into the store - pub fn insert(&self, curve: Curve) -> Result, Infallible> { + pub fn insert( + &self, + handle: Handle, + curve: Curve, + ) -> Result<(), Infallible> { curve.validate()?; - Ok(self.store.insert(curve)) + self.store.insert(handle, curve); + Ok(()) } } @@ -94,13 +104,20 @@ pub struct Cycles { } impl Cycles { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Cycle`] into the store pub fn insert( &self, + handle: Handle, cycle: Cycle, - ) -> Result, CycleValidationError> { + ) -> Result<(), CycleValidationError> { cycle.validate()?; - Ok(self.store.insert(cycle)) + self.store.insert(handle, cycle); + Ok(()) } } @@ -111,13 +128,20 @@ pub struct Faces { } impl Faces { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Face`] into the store pub fn insert( &self, + handle: Handle, face: Face, - ) -> Result, FaceValidationError> { + ) -> Result<(), FaceValidationError> { face.validate()?; - Ok(self.store.insert(face)) + self.store.insert(handle, face); + Ok(()) } } @@ -128,13 +152,20 @@ pub struct GlobalCurves { } impl GlobalCurves { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`GlobalCurve`] into the store pub fn insert( &self, + handle: Handle, global_curve: GlobalCurve, - ) -> Result, Infallible> { + ) -> Result<(), Infallible> { global_curve.validate()?; - Ok(self.store.insert(global_curve)) + self.store.insert(handle, global_curve); + Ok(()) } } @@ -145,13 +176,20 @@ pub struct GlobalEdges { } impl GlobalEdges { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`GlobalEdge`] into the store pub fn insert( &self, + handle: Handle, global_edge: GlobalEdge, - ) -> Result, Infallible> { + ) -> Result<(), Infallible> { global_edge.validate()?; - Ok(self.store.insert(global_edge)) + self.store.insert(handle, global_edge); + Ok(()) } } @@ -162,13 +200,20 @@ pub struct GlobalVertices { } impl GlobalVertices { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`GlobalVertex`] into the store pub fn insert( &self, + handle: Handle, global_vertex: GlobalVertex, - ) -> Result, Infallible> { + ) -> Result<(), Infallible> { global_vertex.validate()?; - Ok(self.store.insert(global_vertex)) + self.store.insert(handle, global_vertex); + Ok(()) } } @@ -179,13 +224,20 @@ pub struct HalfEdges { } impl HalfEdges { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`HalfEdge`] into the store pub fn insert( &self, + handle: Handle, half_edge: HalfEdge, - ) -> Result, HalfEdgeValidationError> { + ) -> Result<(), HalfEdgeValidationError> { half_edge.validate()?; - Ok(self.store.insert(half_edge)) + self.store.insert(handle, half_edge); + Ok(()) } } @@ -196,10 +248,20 @@ pub struct Shells { } impl Shells { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Shell`] into the store - pub fn insert(&self, shell: Shell) -> Result, Infallible> { + pub fn insert( + &self, + handle: Handle, + shell: Shell, + ) -> Result<(), Infallible> { shell.validate()?; - Ok(self.store.insert(shell)) + self.store.insert(handle, shell); + Ok(()) } } @@ -210,10 +272,20 @@ pub struct Sketches { } impl Sketches { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Sketch`] into the store - pub fn insert(&self, sketch: Sketch) -> Result, Infallible> { + pub fn insert( + &self, + handle: Handle, + sketch: Sketch, + ) -> Result<(), Infallible> { sketch.validate()?; - Ok(self.store.insert(sketch)) + self.store.insert(handle, sketch); + Ok(()) } } @@ -224,10 +296,20 @@ pub struct Solids { } impl Solids { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Solid`] into the store - pub fn insert(&self, solid: Solid) -> Result, Infallible> { + pub fn insert( + &self, + handle: Handle, + solid: Solid, + ) -> Result<(), Infallible> { solid.validate()?; - Ok(self.store.insert(solid)) + self.store.insert(handle, solid); + Ok(()) } } @@ -238,13 +320,20 @@ pub struct SurfaceVertices { } impl SurfaceVertices { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`SurfaceVertex`] into the store pub fn insert( &self, + handle: Handle, surface_vertex: SurfaceVertex, - ) -> Result, SurfaceVertexValidationError> { + ) -> Result<(), SurfaceVertexValidationError> { surface_vertex.validate()?; - Ok(self.store.insert(surface_vertex)) + self.store.insert(handle, surface_vertex); + Ok(()) } } @@ -259,13 +348,20 @@ pub struct Surfaces { } impl Surfaces { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Surface`] into the store pub fn insert( &self, + handle: Handle, surface: Surface, - ) -> Result, Infallible> { + ) -> Result<(), Infallible> { surface.validate()?; - Ok(self.store.insert(surface)) + self.store.insert(handle, surface); + Ok(()) } /// Access the xy-plane @@ -286,20 +382,33 @@ impl Surfaces { impl Default for Surfaces { fn default() -> Self { - let store = Store::new(); - - let xy_plane = store.insert(Surface::new(SurfaceGeometry { - u: GlobalPath::x_axis(), - v: Vector::unit_y(), - })); - let xz_plane = store.insert(Surface::new(SurfaceGeometry { - u: GlobalPath::x_axis(), - v: Vector::unit_z(), - })); - let yz_plane = store.insert(Surface::new(SurfaceGeometry { - u: GlobalPath::y_axis(), - v: Vector::unit_z(), - })); + let store: Store = Store::new(); + + let xy_plane = store.reserve(); + store.insert( + xy_plane.clone(), + Surface::new(SurfaceGeometry { + u: GlobalPath::x_axis(), + v: Vector::unit_y(), + }), + ); + + let xz_plane = store.reserve(); + store.insert( + xz_plane.clone(), + Surface::new(SurfaceGeometry { + u: GlobalPath::x_axis(), + v: Vector::unit_z(), + }), + ); + let yz_plane = store.reserve(); + store.insert( + yz_plane.clone(), + Surface::new(SurfaceGeometry { + u: GlobalPath::y_axis(), + v: Vector::unit_z(), + }), + ); Self { store, @@ -317,12 +426,19 @@ pub struct Vertices { } impl Vertices { + /// Reserve a slot for an object in the store + pub fn reserve(&self) -> Handle { + self.store.reserve() + } + /// Insert a [`Vertex`] into the store pub fn insert( &self, + handle: Handle, vertex: Vertex, - ) -> Result, VertexValidationError> { + ) -> Result<(), VertexValidationError> { vertex.validate()?; - Ok(self.store.insert(vertex)) + self.store.insert(handle, vertex); + Ok(()) } } diff --git a/crates/fj-kernel/src/storage/blocks.rs b/crates/fj-kernel/src/storage/blocks.rs index c69b97180..474b3d002 100644 --- a/crates/fj-kernel/src/storage/blocks.rs +++ b/crates/fj-kernel/src/storage/blocks.rs @@ -14,11 +14,6 @@ impl Blocks { } } - pub fn push(&mut self, object: T) -> *const Option { - let (index, _) = self.reserve(); - self.insert(index, object) - } - pub fn reserve(&mut self) -> (Index, *const Option) { let mut current_block = match self.inner.pop() { Some(block) => block, @@ -50,9 +45,9 @@ impl Blocks { ret } - pub fn insert(&mut self, index: Index, object: T) -> *const Option { + pub fn insert(&mut self, index: Index, object: T) { let block = &mut self.inner[index.block_index.0]; - block.insert(index.object_index, object) + block.insert(index.object_index, object); } pub fn get_and_inc(&self, index: &mut Index) -> Option<&Option> { @@ -63,11 +58,6 @@ impl Blocks { Some(object) } - - #[cfg(test)] - pub fn iter(&self) -> impl Iterator + '_ { - self.inner.iter().flat_map(|block| block.iter()) - } } #[derive(Debug)] @@ -101,13 +91,14 @@ impl Block { Ok((index, ptr)) } - pub fn insert( - &mut self, - index: ObjectIndex, - object: T, - ) -> *const Option { - self.objects[index.0] = Some(object); - &self.objects[index.0] + pub fn insert(&mut self, index: ObjectIndex, object: T) { + let slot = &mut self.objects[index.0]; + + if slot.is_some() { + panic!("Attempting to overwrite object in store") + } + + *slot = Some(object); } pub fn get(&self, index: ObjectIndex) -> &Option { @@ -117,21 +108,6 @@ impl Block { pub fn len(&self) -> usize { self.next.0 } - - #[cfg(test)] - pub fn iter(&self) -> impl Iterator + '_ { - let mut i = 0; - iter::from_fn(move || { - if i >= self.len() { - return None; - } - - let object = self.get(ObjectIndex(i)).as_ref()?; - i += 1; - - Some(object) - }) - } } #[derive(Clone, Copy, Debug)] @@ -162,19 +138,3 @@ pub struct BlockIndex(usize); #[derive(Clone, Copy, Debug)] pub struct ObjectIndex(usize); - -#[cfg(test)] -mod tests { - use super::Blocks; - - #[test] - fn push() { - let mut blocks = Blocks::new(1); - - blocks.push(0); - blocks.push(1); - - let objects = blocks.iter().copied().collect::>(); - assert_eq!(objects, [0, 1]); - } -} diff --git a/crates/fj-kernel/src/storage/handle.rs b/crates/fj-kernel/src/storage/handle.rs index 373122a0b..f66f932cf 100644 --- a/crates/fj-kernel/src/storage/handle.rs +++ b/crates/fj-kernel/src/storage/handle.rs @@ -1,6 +1,6 @@ use std::{any::type_name, cmp::Ordering, fmt, hash::Hash, ops::Deref}; -use super::store::StoreInner; +use super::{blocks::Index, store::StoreInner}; /// A handle for an object /// @@ -23,6 +23,7 @@ use super::store::StoreInner; /// comparing the values returned by [`Handle::id`]. pub struct Handle { pub(super) store: StoreInner, + pub(super) index: Index, pub(super) ptr: *const Option, } @@ -85,6 +86,7 @@ impl Clone for Handle { fn clone(&self) -> Self { Self { store: self.store.clone(), + index: self.index, ptr: self.ptr, } } diff --git a/crates/fj-kernel/src/storage/mod.rs b/crates/fj-kernel/src/storage/mod.rs index a19bd1b44..3b73c4b66 100644 --- a/crates/fj-kernel/src/storage/mod.rs +++ b/crates/fj-kernel/src/storage/mod.rs @@ -6,5 +6,5 @@ mod store; pub use self::{ handle::{Handle, HandleWrapper, ObjectId}, - store::{Iter, Reservation, Store}, + store::{Iter, Store}, }; diff --git a/crates/fj-kernel/src/storage/store.rs b/crates/fj-kernel/src/storage/store.rs index 076795b3b..43080bbd6 100644 --- a/crates/fj-kernel/src/storage/store.rs +++ b/crates/fj-kernel/src/storage/store.rs @@ -54,17 +54,40 @@ impl Store { Self { inner } } - /// Insert an object into the store - pub fn insert(&self, object: T) -> Handle { + /// Reserve a slot for an object in the store + /// + /// This method returns a handle to the reserved slot. That handle does not + /// refer to an object yet! Attempting to dereference the handle before it + /// has been used to insert an object into the store, will result in a + /// panic. + /// + /// Usually, you'd acquire one handle, then immediately use it to insert an + /// object using [`Store::insert`]. The methods are separate to support more + /// advanced use cases, like inserting objects that refer to each other. + pub fn reserve(&self) -> Handle { let mut inner = self.inner.write(); - let ptr = inner.blocks.push(object); + + let (index, ptr) = inner.blocks.reserve(); Handle { store: self.inner.clone(), + index, ptr, } } + /// Insert an object into the store + /// + /// # Panics + /// + /// Panics, if the passed `Handle` does not refer to a reserved slot. This + /// can only be the case, if the handle has been used to insert an object + /// before. + pub fn insert(&self, handle: Handle, object: T) { + let mut inner = self.inner.write(); + inner.blocks.insert(handle.index, object); + } + /// Iterate over all objects in this store pub fn iter(&self) -> Iter { Iter { @@ -73,23 +96,6 @@ impl Store { _a: PhantomData, } } - - /// Reserve a slot for an object - /// - /// Returns a [`Reservation`], which can be used to access the [`Handle`] of - /// an object that hasn't been added yet. This makes it possible to use the - /// [`Handle`]'s ID in the construction of the object, or to create groups - /// of objects that reference each other through their [`Handle`]s. - pub fn reserve(&self) -> Reservation { - let mut inner = self.inner.write(); - let (index, ptr) = inner.blocks.reserve(); - - Reservation { - store: self.inner.clone(), - index, - ptr, - } - } } impl Default for Store { @@ -120,50 +126,20 @@ impl<'a, T: 'a> Iterator for Iter<'a, T> { fn next(&mut self) -> Option { let inner = self.store.read(); - let object = inner.blocks.get_and_inc(&mut self.next_index)?; + loop { + let index = self.next_index; + let ptr = inner.blocks.get_and_inc(&mut self.next_index)?; - Some(Handle { - store: self.store.clone(), - ptr: object, - }) - } -} + if ptr.is_none() { + // This is a reserved slot. + continue; + } -/// A reservation of a slot for an object within a [`Store`] -/// -/// See [`Store::reserve`]. -#[derive(Debug)] -pub struct Reservation { - store: StoreInner, - ptr: *const Option, - index: Index, -} - -impl Reservation { - /// Access the [`Handle`] for this reservation - /// - /// You **must not** dereference the handle to access the object it - /// references, until you initialized that object by calling - /// [`Reservation::complete`]. Doing otherwise will lead to a panic. - pub fn handle(&self) -> Handle { - Handle { - store: self.store.clone(), - ptr: self.ptr, - } - } - - /// Complete the reservation by providing an object - /// - /// This method consumes the reservation. After calling it, you can use any - /// [`Handle`]s you acquired from [`Reservation::handle`] without - /// limitations. - pub fn complete(self, object: T) -> Handle { - let mut inner = self.store.write(); - inner.blocks.insert(self.index, object); - - Handle { - store: self.store.clone(), - ptr: self.ptr, + return Some(Handle { + store: self.store.clone(), + index, + ptr, + }); } } } @@ -177,14 +153,18 @@ pub struct StoreInnerInner { #[cfg(test)] mod tests { + use crate::storage::Handle; + use super::Store; #[test] fn insert_and_handle() { let store = Store::with_block_size(1); + let handle: Handle = store.reserve(); let object = 0; - let handle = store.insert(object); + + store.insert(handle.clone(), object); assert_eq!(*handle, object); } @@ -193,31 +173,12 @@ mod tests { fn insert_and_iter() { let store = Store::with_block_size(1); - let a = store.insert(0); - let b = store.insert(1); - - let objects = store.iter().collect::>(); - assert_eq!(objects, [a, b]) - } - - #[test] - fn reserve() { - let store = Store::::new(); - - let a = store.reserve(); + let a: Handle = store.reserve(); let b = store.reserve(); - - let id_a = a.handle().id(); - let id_b = b.handle().id(); - assert_ne!(id_a, id_b); - - let a = a.complete(0); - let b = b.complete(1); - - assert_eq!(*a, 0); - assert_eq!(*b, 1); + store.insert(a.clone(), 0); + store.insert(b.clone(), 1); let objects = store.iter().collect::>(); - assert_eq!(objects, [a, b]); + assert_eq!(objects, [a, b]) } } diff --git a/crates/fj-operations/src/lib.rs b/crates/fj-operations/src/lib.rs index a74e6107a..1dc50ef78 100644 --- a/crates/fj-operations/src/lib.rs +++ b/crates/fj-operations/src/lib.rs @@ -15,6 +15,11 @@ //! [`fj`]: https://crates.io/crates/fj #![warn(missing_docs)] +// I've made a simple change that put `ValidationError` over the threshold for +// this warning. I couldn't come up with an easy fix, and figured that silencing +// the warning is the most practical solution for now, as the validation +// infrastructure is in flux anyway. Maybe the problem will take care of itself. +#![allow(clippy::result_large_err)] pub mod shape_processor;