From 99e66ce681e534ffd57e0dfb1251e4085c8d5392 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 16:41:59 +0100 Subject: [PATCH 1/6] Add `Handle::get` --- src/kernel/shape/handle.rs | 10 ++++++++++ src/kernel/shapes/difference_2d.rs | 2 +- src/kernel/shapes/union.rs | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index b3bfa5d6a..e33156bcf 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -30,6 +30,16 @@ impl Clone for Storage { #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Handle(Storage); +impl Handle { + /// Access the object that the handle references + /// + /// `Handle` also implements `Deref`, but as that can be inconvenient to use + /// in some cases, this method is an inherent proxy for that. + pub fn get(&self) -> &T { + self.0.deref() + } +} + impl Deref for Handle { type Target = T; diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index c6ea0cd69..84ba64cb7 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -53,7 +53,7 @@ impl ToShape for fj::Difference2d { [&mut a, &mut b].map(|shape| shape.faces().all().next().unwrap()); let (cycles_a, cycles_b, surface_a, surface_b) = - match ((*face_a).clone(), (*face_b).clone()) { + match (face_a.get().clone(), face_b.get().clone()) { ( Face::Face { cycles: a, diff --git a/src/kernel/shapes/union.rs b/src/kernel/shapes/union.rs index 8147510fc..a843cca7e 100644 --- a/src/kernel/shapes/union.rs +++ b/src/kernel/shapes/union.rs @@ -19,10 +19,10 @@ impl ToShape for fj::Union { // See issue: // https://github.com/hannobraun/Fornjot/issues/42 for face in a.faces().all() { - shape.faces().add((*face).clone()); + shape.faces().add(face.get().clone()); } for face in b.faces().all() { - shape.faces().add((*face).clone()); + shape.faces().add(face.get().clone()); } shape From d54869d65f2e65294d9d93a61aaa33080e66e200 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:37:33 +0100 Subject: [PATCH 2/6] Document `Handle` --- src/kernel/shape/handle.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index e33156bcf..9fc0c1d1b 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -27,6 +27,23 @@ impl Clone for Storage { } } +/// A handle to an object stored within [`Shape`] +/// +/// If an object of type `T` (this could be `Curve`, `Vertex`, etc.) is added to +/// `Shape`, a `Handle` is returned. This handle is then used in topological +/// types to refer to the object, instead of having those types own an instance +/// of the object. +/// +/// This approach has two advantages: +/// +/// 1. The object can't be mutated through the handle. Since an object can be +/// referred to by multiple other objects, mutating it locally would have no +/// effect on those other references. `Handle` preventing that removes this +/// source of errors. +/// 2. The object is guaranteed to be in `Shape`, as `Handle`s can't be created +/// any other way. This means that if the `Shape` needs to be modified, any +/// objects can be updated once, without requiring an update of all the other +/// objects that reference it. #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Handle(Storage); From fed7b377ea3b70966c1452945343c84a94f00ec4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:37:40 +0100 Subject: [PATCH 3/6] Add comment --- src/kernel/shape/handle.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index 9fc0c1d1b..6748e054d 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -21,6 +21,9 @@ impl Deref for Storage { } } +// Deriving `Clone` would only derive `Clone` where `T: Clone`. This +// implementation doesn't have that limitation, providing `Clone` for all +// `Handle`s instead. impl Clone for Storage { fn clone(&self) -> Self { Self(self.0.clone()) From a6a25b798ad4ab366a61c04ed1a12d8a277e882b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:38:01 +0100 Subject: [PATCH 4/6] Change order of code `Handle` is the important type in this module. It should come first. --- src/kernel/shape/handle.rs | 60 +++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index 6748e054d..6947a8956 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -1,35 +1,5 @@ use std::{hash::Hash, ops::Deref, rc::Rc}; -#[derive(Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] -pub struct Storage(Rc); - -impl Storage { - pub(super) fn new(value: T) -> Self { - Self(Rc::new(value)) - } - - pub(super) fn handle(&self) -> Handle { - Handle(self.clone()) - } -} - -impl Deref for Storage { - type Target = T; - - fn deref(&self) -> &Self::Target { - self.0.deref() - } -} - -// Deriving `Clone` would only derive `Clone` where `T: Clone`. This -// implementation doesn't have that limitation, providing `Clone` for all -// `Handle`s instead. -impl Clone for Storage { - fn clone(&self) -> Self { - Self(self.0.clone()) - } -} - /// A handle to an object stored within [`Shape`] /// /// If an object of type `T` (this could be `Curve`, `Vertex`, etc.) is added to @@ -67,3 +37,33 @@ impl Deref for Handle { self.0.deref() } } + +#[derive(Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub struct Storage(Rc); + +impl Storage { + pub(super) fn new(value: T) -> Self { + Self(Rc::new(value)) + } + + pub(super) fn handle(&self) -> Handle { + Handle(self.clone()) + } +} + +impl Deref for Storage { + type Target = T; + + fn deref(&self) -> &Self::Target { + self.0.deref() + } +} + +// Deriving `Clone` would only derive `Clone` where `T: Clone`. This +// implementation doesn't have that limitation, providing `Clone` for all +// `Handle`s instead. +impl Clone for Storage { + fn clone(&self) -> Self { + Self(self.0.clone()) + } +} From f408a2ec77525be536509d71c58782e460bdf3cd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:38:39 +0100 Subject: [PATCH 5/6] Reduce visibility of `Storage` It is not intended to be used outside of `Shape`. --- src/kernel/shape/handle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index 6947a8956..99e8ae120 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -39,7 +39,7 @@ impl Deref for Handle { } #[derive(Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] -pub struct Storage(Rc); +pub(super) struct Storage(Rc); impl Storage { pub(super) fn new(value: T) -> Self { From 00f59db483315051875dd89b481bef371bfb7aff Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:41:17 +0100 Subject: [PATCH 6/6] Document `Storage` --- src/kernel/shape/handle.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/kernel/shape/handle.rs b/src/kernel/shape/handle.rs index 99e8ae120..ba2b4fa21 100644 --- a/src/kernel/shape/handle.rs +++ b/src/kernel/shape/handle.rs @@ -38,14 +38,17 @@ impl Deref for Handle { } } +/// Internal type used in collections within [`Shape`] #[derive(Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub(super) struct Storage(Rc); impl Storage { + /// Create a [`Storage`] instance that wraps the provided object pub(super) fn new(value: T) -> Self { Self(Rc::new(value)) } + /// Create a handle that refers to this [`Storage`] instance pub(super) fn handle(&self) -> Handle { Handle(self.clone()) }