From 8be7b0cdce861ec32cdeb58b29a099ef3664d2de Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 12:05:26 +0100 Subject: [PATCH 01/15] Update documentation of `Handle` --- crates/fj-core/src/storage/handle.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index cbcc85aab..0672c3cb8 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -4,18 +4,18 @@ 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 +/// ## Equality and Identity /// -/// Equality of `Handle`s is defined via the objects they reference. If those -/// objects are equal, the `Handle`s are considered equal. +/// Equality of `Handle`s is defined by the equality of the stored objects they +/// reference. If those objects are equal, the `Handle`s are considered equal. /// -/// This is distinct from the *identity* of the referenced objects. Two objects +/// This is distinct from the *identity* of the stored 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 @@ -30,12 +30,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, From 2faebbe7859321b84fe9afc96c75122348acf768 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 12:11:12 +0100 Subject: [PATCH 02/15] Document meaning of bare and stored objects --- crates/fj-core/src/storage/handle.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index 0672c3cb8..09a6f10a2 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -10,6 +10,17 @@ use super::{blocks::Index, store::StoreInner}; /// handle dereferences to the object it points to, via its [`Deref`] /// implementation. /// +/// ## Bare objects and stored objects +/// +/// 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`. +/// +/// 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. +/// /// ## Equality and Identity /// /// Equality of `Handle`s is defined by the equality of the stored objects they From e39157df902b528cb39a3af77a0ef23a961f45ff Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 12:18:21 +0100 Subject: [PATCH 03/15] Consolidate documentation on object identity --- crates/fj-core/src/objects/mod.rs | 27 ----------------------- crates/fj-core/src/storage/handle.rs | 32 ++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 36 deletions(-) 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/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index 09a6f10a2..d31155062 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -23,17 +23,31 @@ use super::{blocks::Index, store::StoreInner}; /// /// ## Equality and Identity /// -/// Equality of `Handle`s is defined by the equality of the stored objects they -/// reference. If those objects are equal, the `Handle`s are considered equal. +/// 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. /// -/// This is distinct from the *identity* of the stored 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. +/// However, that two objects are *equal* does not mean they are *identical*. /// -/// You can compare the identity of two objects through their `Handle`s, by -/// comparing the values returned by [`Handle::id`]. +/// 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, From f97f2620ab9837725d2c34fd08a3dda144708b1a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 12:19:42 +0100 Subject: [PATCH 04/15] Update documentation of `ObjectId` --- crates/fj-core/src/storage/handle.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index d31155062..33de03d33 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -197,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); From e27a45a668f19a8f4616d77db882b89c738c17fb Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 12:32:10 +0100 Subject: [PATCH 05/15] Update documentation of `HandleWrapper` --- crates/fj-core/src/storage/handle.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index 33de03d33..957772929 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -217,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 { From 8fbdc70f336987873fa1f076e351330dfb589e20 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jan 2024 13:22:15 +0100 Subject: [PATCH 06/15] Update documentation of `IsObject` --- crates/fj-core/src/objects/is_object.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 From d6cdb76d0426640aadcfcb41bbbbf62fabd03a92 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:17:00 +0100 Subject: [PATCH 07/15] Fix typo in doc comment --- crates/fj-core/src/objects/object_set.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index f8928e0d3..ed78cf694 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 /// From b5cf8d5996ebaf2c6eac75e4da86a70167e1f1c9 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:20:32 +0100 Subject: [PATCH 08/15] Update comment --- crates/fj-core/src/objects/object_set.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index ed78cf694..3f4a7a211 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -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 From 0c77d82da2402026e8e47e8337605f235743c5b5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:23:44 +0100 Subject: [PATCH 09/15] Improve wording in `Curve` documentation --- crates/fj-core/src/objects/kinds/curve.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 37e8f70d8fb1f13a43b445f3ed6c4a08f28c8ae7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:24:38 +0100 Subject: [PATCH 10/15] Update comment --- crates/fj-core/src/objects/kinds/region.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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()) } From 91121cf6ed915d4d3738d70d8680c048517f4c78 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:25:33 +0100 Subject: [PATCH 11/15] Improve wording in `Vertex` documentation --- crates/fj-core/src/objects/kinds/vertex.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From b7b11e52a06d6ee7f957a17a8b915bb2e32f3c05 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:26:57 +0100 Subject: [PATCH 12/15] Improve wording in `operations` documentation --- crates/fj-core/src/operations/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 From a52a7d26e3d8fdb15743749c473add0c46fa1668 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:30:35 +0100 Subject: [PATCH 13/15] Fix doc comment --- crates/fj-core/src/operations/replace/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, From 2b6ab476de9f57544712e2d51ef6c03bc65ac0c7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:37:57 +0100 Subject: [PATCH 14/15] Improve wording in comment --- crates/fj-core/src/storage/handle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index 957772929..b6d99f3b6 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -102,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") } From aa5ec439b331ecccf8eaa3464a82c8eecc07bd9c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 31 Jan 2024 10:42:21 +0100 Subject: [PATCH 15/15] Update documentation of `Store` --- crates/fj-core/src/storage/store.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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();