diff --git a/crates/fj-core/src/objects/is_object.rs b/crates/fj-core/src/objects/is_object.rs index 0c1328083..0e6943cde 100644 --- a/crates/fj-core/src/objects/is_object.rs +++ b/crates/fj-core/src/objects/is_object.rs @@ -6,9 +6,9 @@ use super::{ /// A trait implemented for all object types /// -/// This trait is implemented for both `T` and `Handle`, where `T` is the -/// type of any bare object. The `BareObject` associated type provides access to -/// the bare object type. +/// This trait is implemented for both bare objects (`T`) and stored objects +/// (`Handle`). The `BareObject` associated type provides access to the bare +/// object type. /// /// This is a piece of infrastructure that is useful for other traits, which /// would otherwise have to duplicate its functionality. Users are unlikely to diff --git a/crates/fj-core/src/objects/kinds/curve.rs b/crates/fj-core/src/objects/kinds/curve.rs index f68676e32..de4445255 100644 --- a/crates/fj-core/src/objects/kinds/curve.rs +++ b/crates/fj-core/src/objects/kinds/curve.rs @@ -9,7 +9,7 @@ /// /// # Equality /// -/// `Curve` contains no data and exists purely to be used within a `Handle`, +/// `Curve` contains no data and exists purely to be referenced via a `Handle`, /// where `Handle::id` can be used to compare different instances of `Curve`. /// /// If `Curve` had `Eq`/`PartialEq` implementations, it containing no data would diff --git a/crates/fj-core/src/objects/kinds/region.rs b/crates/fj-core/src/objects/kinds/region.rs index c5ccb475b..b106c928f 100644 --- a/crates/fj-core/src/objects/kinds/region.rs +++ b/crates/fj-core/src/objects/kinds/region.rs @@ -50,8 +50,8 @@ impl Region { /// Access all cycles of the region (both exterior and interior) pub fn all_cycles(&self) -> impl Iterator> { - // It would be nice to return `&Handles` here, but I don't see a way for - // doing that here *and* in `interiors`. + // It would be nice to return `&ObjectSet` here, but I don't see a way + // for doing that here *and* in `interiors`. [self.exterior()].into_iter().chain(self.interiors()) } diff --git a/crates/fj-core/src/objects/kinds/vertex.rs b/crates/fj-core/src/objects/kinds/vertex.rs index 8fc398ee8..4d9884ed9 100644 --- a/crates/fj-core/src/objects/kinds/vertex.rs +++ b/crates/fj-core/src/objects/kinds/vertex.rs @@ -67,7 +67,7 @@ /// /// ## Equality /// -/// `Vertex` contains no data and exists purely to be used within a `Handle`, +/// `Vertex` contains no data and exists purely to be referenced via a `Handle`, /// where `Handle::id` can be used to compare different instances of `Vertex`. /// /// If `Vertex` had `Eq`/`PartialEq` implementations, it containing no data diff --git a/crates/fj-core/src/objects/mod.rs b/crates/fj-core/src/objects/mod.rs index db4110aee..27babb14c 100644 --- a/crates/fj-core/src/objects/mod.rs +++ b/crates/fj-core/src/objects/mod.rs @@ -10,33 +10,6 @@ //! All objects are stored in centralized storage (see [`Objects`]) and referred //! to through a [`Handle`]. //! -//! -//! ## Object Equality vs Object Identity -//! -//! Most objects have [`Eq`]/[`PartialEq`] implementations that can be used to -//! determine equality. Those implementations are derived, meaning two objects -//! are equal, if all of their fields are equal. This can be used to compare -//! objects structurally. [`Handle`]'s own [`Eq`]/[`PartialEq`] implementations -//! defer to those of the object it references. -//! -//! However, that two objects are *equal* does not mean they are *identical*. -//! See [`Handle`]'s documentation for an explanation of this distinction. -//! -//! This distinction is relevant, because non-identical objects that are -//! *supposed* to be equal can end up being equal, if they are created based on -//! simple input data (as you might have in a unit test). But they might end up -//! slightly different, if they are created based on complex input data (as you -//! might have in the real world). This situation would most likely result in a -//! bug that is not easily caught in testing. -//! -//! ### Validation Must Use Identity -//! -//! To prevent such situations, where everything looked fine during development, -//! but you end up with a bug in production, any validation code that compares -//! objects and expects them to be the same, must do that comparison based on -//! identity, not equality. That way, this problem can never happen, because we -//! never expect non-identical objects to be the same. -//! //! [`Handle`]: crate::storage::Handle mod any_object; diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index f8928e0d3..3f4a7a211 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -23,7 +23,7 @@ pub struct ObjectSet { } impl ObjectSet { - /// Create an instances of `ObjectSet` from an iterator over `Handle` + /// Create an instance of `ObjectSet` from an iterator over `Handle` /// /// # Panics /// @@ -209,8 +209,8 @@ impl IntoIterator for ObjectSet { } impl<'r, T> IntoIterator for &'r ObjectSet { - // You might wonder why we're returning references to handles here, when - // `Handle` already is kind of reference, and easily cloned. + // You might wonder why we're returning references to `Handle`s here, when + // `Handle` already is a kind of reference, and easily cloned. // // Most of the time that doesn't make a difference, but there are use cases // where dealing with owned `Handle`s is inconvenient, for example when diff --git a/crates/fj-core/src/operations/mod.rs b/crates/fj-core/src/operations/mod.rs index 730f3ded7..fa138c719 100644 --- a/crates/fj-core/src/operations/mod.rs +++ b/crates/fj-core/src/operations/mod.rs @@ -15,12 +15,12 @@ //! misunderstanding this principle. //! //! -//! ### Bare Objects vs `Handle` +//! ### Bare Objects vs Stored Objects //! //! Extension traits are mostly implemented for bare object types (i.e. for //! `Vertex` instead of `Handle`). This makes those operations more -//! flexible, as a `Handle` might not be available, but a bare object can always -//! be produced from a `Handle`. +//! flexible, as a `Handle` might not be available, but a reference to the bare +//! object can always be produced from a `Handle`. //! //! They also mostly return bare objects, which also provides more flexibility //! to the user. An inserted object must always be valid, but in a series of diff --git a/crates/fj-core/src/operations/replace/mod.rs b/crates/fj-core/src/operations/replace/mod.rs index 0de989182..9ea13048a 100644 --- a/crates/fj-core/src/operations/replace/mod.rs +++ b/crates/fj-core/src/operations/replace/mod.rs @@ -154,7 +154,7 @@ impl ReplaceOutput { } impl ReplaceOutput { - /// Return the original object, or insert the updated on and return handle + /// Return the wrapped object, whether it's the original or was updated pub fn into_inner(self) -> T { match self { Self::Original(inner) => inner, diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index cbcc85aab..b6d99f3b6 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -4,25 +4,50 @@ use std::{ use super::{blocks::Index, store::StoreInner}; -/// A handle for an object +/// # A handle that references a stored object /// /// You can get an instance of `Handle` by inserting an object into a store. A /// handle dereferences to the object it points to, via its [`Deref`] /// implementation. /// -/// # Equality and Identity +/// ## Bare objects and stored objects /// -/// Equality of `Handle`s is defined via the objects they reference. If those -/// objects are equal, the `Handle`s are considered equal. +/// A bare object is just that: an instance of a bare object type. Once a bare +/// objects is inserted into storage, it becomes a stored object. A stored +/// object is owned by the store, and can be referenced through instances of +/// `Handle`. /// -/// This is distinct from the *identity* of the referenced objects. Two objects -/// might be equal, but they might be have been created at different times, for -/// different reasons, and thus live in different slots in the storage. This is -/// a relevant distinction when validating objects, as equal but not identical -/// objects might be a sign of a bug. +/// The point of doing this, is to provide objects with a unique identity, via +/// their location within storage. The importance of this is expanded upon in +/// the next section. /// -/// You can compare the identity of two objects through their `Handle`s, by -/// comparing the values returned by [`Handle::id`]. +/// ## Equality and Identity +/// +/// Most objects have [`Eq`]/[`PartialEq`] implementations that can be used to +/// determine equality. Those implementations are derived, meaning two objects +/// are equal, if all of their fields are equal. This can be used to compare +/// objects structurally. [`Handle`]'s own [`Eq`]/[`PartialEq`] implementations +/// defer to those of the stored object it references. +/// +/// However, that two objects are *equal* does not mean they are *identical*. +/// +/// This distinction is relevant, because non-identical objects that are +/// *supposed* to be equal can in fact end up equal, if they are created based +/// on simple input data (as you might have in a unit test). But they might end +/// up slightly different, if they are created based on complex input data (as +/// you might have in a real-world scenario). This situation would most likely +/// result in a bug that is not easily caught in testing. +/// +/// You can compare the identity of two `Handle`s, by comparing the values +/// returned by [`Handle::id`]. +/// +/// ### Validation Must Use Identity +/// +/// To prevent situations where everything looks fine during development, but +/// you end up with a bug in production, any validation code that compares +/// objects and expects them to be the same, must do that comparison based on +/// identity, not equality. That way, this problem can never happen, because we +/// never expect non-identical objects to be equal. pub struct Handle { pub(super) store: StoreInner, pub(super) index: Index, @@ -30,12 +55,12 @@ pub struct Handle { } impl Handle { - /// Access this pointer's unique id + /// Access the object's unique id pub fn id(&self) -> ObjectId { ObjectId::from_ptr(self.ptr) } - /// Return a clone of the object this handle refers to + /// Return a bare object, which is a clone of the referenced stored object pub fn clone_object(&self) -> T where T: Clone, @@ -77,8 +102,8 @@ impl Deref for Handle { // which I've run successfully under Miri. let slot = unsafe { &*self.ptr }; - // Can only panic, if the object has been reserved, but the reservation - // was never completed. + // Can only panic, if the object was reserved, but the reservation has + // never been completed. slot.as_ref() .expect("Handle references non-existing object") } @@ -172,9 +197,10 @@ impl From> for Handle { unsafe impl Send for Handle {} unsafe impl Sync for Handle {} -/// Represents the ID of an object +/// The unique ID of a stored object /// -/// See [`Handle::id`]. +/// You can access a stored object's ID via [`Handle::id`]. Please refer to the +/// documentation of [`Handle`] for an explanation of object identity. #[derive(Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct ObjectId(pub(crate) u64); @@ -191,22 +217,22 @@ impl fmt::Debug for ObjectId { } } -/// A wrapper around [`Handle`] to define equality based on identity +/// A wrapper around [`Handle`] that defines equality based on identity /// -/// This is a utility type that implements [`Eq`]/[`PartialEq`] and other common -/// traits that are based on those, based on the identity of object that the -/// wrapped handle references. This is useful, if a type of object doesn't -/// implement `Eq`/`PartialEq`, which means handles referencing it won't -/// implement those types either. +/// `HandleWrapper` implements [`Eq`]/[`PartialEq`] and other common traits +/// that are based on those, based on the identity of a stored object that the +/// wrapped [`Handle`] references. /// -/// Typically, if an object doesn't implement [`Eq`]/[`PartialEq`], it will do -/// so for good reason. If you need something that represents the object and -/// implements those missing traits, you might want to be explicit about what -/// you're doing, and access its ID via [`Handle::id`] instead. +/// This is useful, since some objects are empty (meaning, they don't contain +/// any data, and don't reference other objects). Such objects only exist to be +/// distinguished based on their identity. But since a bare object doesn't have +/// an identity yet, there's no meaningful way to implement [`Eq`]/[`PartialEq`] +/// for such a bare object type. /// -/// But if you have a struct that owns a [`Handle`] to such an object, and you -/// want to be able to derive various traits that are not available for the -/// [`Handle`] itself, this wrapper is for you. +/// However, such objects are referenced by other objects, and if we want to +/// derive [`Eq`]/[`PartialEq`] for a referencing object, we need something that +/// can provide [`Eq`]/[`PartialEq`] implementations for the empty objects. That +/// is the purpose of `HandleWrapper`. pub struct HandleWrapper(pub Handle); impl Deref for HandleWrapper { diff --git a/crates/fj-core/src/storage/store.rs b/crates/fj-core/src/storage/store.rs index 44307f324..3e2a4029e 100644 --- a/crates/fj-core/src/storage/store.rs +++ b/crates/fj-core/src/storage/store.rs @@ -56,14 +56,14 @@ impl Store { /// 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. + /// This method returns a [`Handle`] that references 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 + /// Usually, you'd acquire a `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. + /// advanced use cases, like inserting objects that reference each other. pub fn reserve(&self) -> Handle { let mut inner = self.inner.write();