From 6e29d0fc6ea0d4fddb804a4ec43abad669d5f00d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:20:10 +0100 Subject: [PATCH 1/6] Add `ObjectSetIter` --- crates/fj-core/src/objects/mod.rs | 2 +- crates/fj-core/src/objects/object_set.rs | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/fj-core/src/objects/mod.rs b/crates/fj-core/src/objects/mod.rs index 27babb14ce..f3e186d777 100644 --- a/crates/fj-core/src/objects/mod.rs +++ b/crates/fj-core/src/objects/mod.rs @@ -33,6 +33,6 @@ pub use self::{ surface::Surface, vertex::Vertex, }, - object_set::ObjectSet, + object_set::{ObjectSet, ObjectSetIter}, stores::{Objects, Surfaces}, }; diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index 3f4a7a2118..b766aba74a 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -130,7 +130,7 @@ impl ObjectSet { } /// Access an iterator over the objects - pub fn iter(&self) -> slice::Iter> { + pub fn iter(&self) -> ObjectSetIter { self.inner.iter() } @@ -218,9 +218,14 @@ impl<'r, T> IntoIterator for &'r ObjectSet { // an adapter's closure, if you own that argument. You can, if you just // reference the argument. type Item = &'r Handle; - type IntoIter = slice::Iter<'r, Handle>; + type IntoIter = ObjectSetIter<'r, T>; fn into_iter(self) -> Self::IntoIter { self.iter() } } + +/// An borrowed iterator over an [`ObjectSet`] +/// +/// See [`ObjectSet::iter`]. +pub type ObjectSetIter<'r, T> = slice::Iter<'r, Handle>; From 86a4fd15f69bb670ab005378ce6068f9a04a1c12 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:20:25 +0100 Subject: [PATCH 2/6] Reorder code to improve clarity --- crates/fj-core/src/objects/object_set.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index b766aba74a..4d3f32e827 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -199,15 +199,6 @@ where } } -impl IntoIterator for ObjectSet { - type Item = Handle; - type IntoIter = vec::IntoIter>; - - fn into_iter(self) -> Self::IntoIter { - self.inner.into_iter() - } -} - impl<'r, T> IntoIterator for &'r ObjectSet { // You might wonder why we're returning references to `Handle`s here, when // `Handle` already is a kind of reference, and easily cloned. @@ -225,6 +216,15 @@ impl<'r, T> IntoIterator for &'r ObjectSet { } } +impl IntoIterator for ObjectSet { + type Item = Handle; + type IntoIter = vec::IntoIter>; + + fn into_iter(self) -> Self::IntoIter { + self.inner.into_iter() + } +} + /// An borrowed iterator over an [`ObjectSet`] /// /// See [`ObjectSet::iter`]. From e0a9a0b543756a4cfffbd3b1554251ab427346ae Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:21:28 +0100 Subject: [PATCH 3/6] Add `ObjectSetIntoIter` --- crates/fj-core/src/objects/mod.rs | 2 +- crates/fj-core/src/objects/object_set.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/objects/mod.rs b/crates/fj-core/src/objects/mod.rs index f3e186d777..1b6bed7a91 100644 --- a/crates/fj-core/src/objects/mod.rs +++ b/crates/fj-core/src/objects/mod.rs @@ -33,6 +33,6 @@ pub use self::{ surface::Surface, vertex::Vertex, }, - object_set::{ObjectSet, ObjectSetIter}, + object_set::{ObjectSet, ObjectSetIntoIter, ObjectSetIter}, stores::{Objects, Surfaces}, }; diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index 4d3f32e827..88dba0acd8 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -218,7 +218,7 @@ impl<'r, T> IntoIterator for &'r ObjectSet { impl IntoIterator for ObjectSet { type Item = Handle; - type IntoIter = vec::IntoIter>; + type IntoIter = ObjectSetIntoIter; fn into_iter(self) -> Self::IntoIter { self.inner.into_iter() @@ -229,3 +229,6 @@ impl IntoIterator for ObjectSet { /// /// See [`ObjectSet::iter`]. pub type ObjectSetIter<'r, T> = slice::Iter<'r, Handle>; + +/// An owned iterator over an [`ObjectSet`] +pub type ObjectSetIntoIter = vec::IntoIter>; From 3dbc60e255d8ec9a6c4500cc923b45e3b6990f78 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:09:05 +0100 Subject: [PATCH 4/6] Move comment into documentation --- crates/fj-core/src/objects/object_set.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index 88dba0acd8..c4ced2bbd5 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -200,14 +200,6 @@ where } impl<'r, T> IntoIterator for &'r ObjectSet { - // 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 - // using iterator adapters. You can't return a reference to the argument of - // an adapter's closure, if you own that argument. You can, if you just - // reference the argument. type Item = &'r Handle; type IntoIter = ObjectSetIter<'r, T>; @@ -231,4 +223,13 @@ impl IntoIterator for ObjectSet { pub type ObjectSetIter<'r, T> = slice::Iter<'r, Handle>; /// An owned iterator over an [`ObjectSet`] +/// +/// 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 using +/// iterator adapters. You can't return a reference to the argument of an +/// adapter's closure, if you own that argument. You can, if you just reference +/// the argument. pub type ObjectSetIntoIter = vec::IntoIter>; From 8d3c01623889f3b762a030d8e4d7eff5e68fdf7e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:34:36 +0100 Subject: [PATCH 5/6] Add conversion methods for `HandleWrapper` --- crates/fj-core/src/storage/handle.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/fj-core/src/storage/handle.rs b/crates/fj-core/src/storage/handle.rs index b6d99f3b60..9c2a9420c9 100644 --- a/crates/fj-core/src/storage/handle.rs +++ b/crates/fj-core/src/storage/handle.rs @@ -235,6 +235,18 @@ impl fmt::Debug for ObjectId { /// is the purpose of `HandleWrapper`. pub struct HandleWrapper(pub Handle); +impl HandleWrapper { + /// Convert `&self` into a `&Handle` + pub fn as_handle(&self) -> &Handle { + &self.0 + } + + /// Convert `self` into a `Handle` + pub fn into_handle(self) -> Handle { + self.0 + } +} + impl Deref for HandleWrapper { type Target = Handle; From 64b5cfc6485e46c08b2944e7036e271921989a39 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 20 Feb 2024 12:35:00 +0100 Subject: [PATCH 6/6] Fix panic due to equal but not identical objects --- crates/fj-core/src/objects/object_set.rs | 44 +++++++++++++++++++----- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/crates/fj-core/src/objects/object_set.rs b/crates/fj-core/src/objects/object_set.rs index c4ced2bbd5..9fb8f61d5a 100644 --- a/crates/fj-core/src/objects/object_set.rs +++ b/crates/fj-core/src/objects/object_set.rs @@ -1,8 +1,8 @@ -use std::{collections::BTreeSet, fmt::Debug, slice, vec}; +use std::{collections::BTreeSet, fmt::Debug, iter, slice, vec}; use itertools::Itertools; -use crate::storage::Handle; +use crate::storage::{Handle, HandleWrapper}; /// An ordered set of objects /// @@ -19,7 +19,7 @@ pub struct ObjectSet { // structure (since it is used in objects, and objects themselves are // immutable). We need to make sure there are no duplicates when this is // constructed (see the constructor below), but after that, we're fine. - inner: Vec>, + inner: Vec>, } impl ObjectSet { @@ -36,6 +36,8 @@ impl ObjectSet { let mut inner = Vec::new(); for handle in handles { + let handle = HandleWrapper::from(handle); + if added.contains(&handle) { panic!( "Constructing `ObjectSet` with duplicate handle: {:?}", @@ -97,7 +99,7 @@ impl ObjectSet { /// Return the n-th item pub fn nth(&self, index: usize) -> Option<&Handle> { - self.inner.get(index) + self.inner.get(index).map(|wrapper| &wrapper.0) } /// Return the n-th item, treating the index space as circular @@ -131,7 +133,7 @@ impl ObjectSet { /// Access an iterator over the objects pub fn iter(&self) -> ObjectSetIter { - self.inner.iter() + self.inner.iter().map(HandleWrapper::as_handle) } /// Access an iterator over the neighboring pairs of all contained objects @@ -213,14 +215,17 @@ impl IntoIterator for ObjectSet { type IntoIter = ObjectSetIntoIter; fn into_iter(self) -> Self::IntoIter { - self.inner.into_iter() + self.inner.into_iter().map(HandleWrapper::into_handle) } } /// An borrowed iterator over an [`ObjectSet`] /// /// See [`ObjectSet::iter`]. -pub type ObjectSetIter<'r, T> = slice::Iter<'r, Handle>; +pub type ObjectSetIter<'r, T> = iter::Map< + slice::Iter<'r, HandleWrapper>, + fn(&HandleWrapper) -> &Handle, +>; /// An owned iterator over an [`ObjectSet`] /// @@ -232,4 +237,27 @@ pub type ObjectSetIter<'r, T> = slice::Iter<'r, Handle>; /// iterator adapters. You can't return a reference to the argument of an /// adapter's closure, if you own that argument. You can, if you just reference /// the argument. -pub type ObjectSetIntoIter = vec::IntoIter>; +pub type ObjectSetIntoIter = iter::Map< + vec::IntoIter>, + fn(HandleWrapper) -> Handle, +>; + +#[cfg(test)] +mod tests { + use crate::{objects::Cycle, operations::insert::Insert, Core}; + + use super::ObjectSet; + + #[test] + fn equal_but_not_identical_objects() { + let mut core = Core::new(); + + let bare_cycle = Cycle::new([]); + let cycle_a = bare_cycle.clone().insert(&mut core); + let cycle_b = bare_cycle.insert(&mut core); + + let _object_set = ObjectSet::new([cycle_a, cycle_b]); + // Nothing more to test. `ObjectSet` panics on duplicate objects, and it + // shouldn't do that in this case. + } +}