Merge pull request #33 from luxbe/trivial_bounds

Implement debug trait with nightly trivial_bounds feature

.github/workflows/rust.yml → .github/workflows/ci.yml

@@ -1,4 +1,4 @@
-name: Rust
+name: CI
 
 on:
   push:
@@ -20,3 +20,6 @@ jobs:
       run: cargo build --verbose
     - name: Run tests
       run: cargo test --verbose
+    - name: cargo-semver-checks
+      uses: obi1kenobi/cargo-semver-checks-action@v2.1
+

README.md

@@ -1,4 +1,4 @@
-# MultiIndexMap [![Tests](https://github.com/lun3x/multi_index_map/actions/workflows/rust.yml/badge.svg?branch=master)](https://github.com/lun3x/multi_index_map/actions/workflows/rust.yml)
+# MultiIndexMap [![Tests](https://github.com/lun3x/multi_index_map/actions/workflows/ci.yml/badge.svg?branch=master)](https://github.com/lun3x/multi_index_map/actions/workflows/ci.yml)
 
 [Also available on crates.io.](https://crates.io/crates/multi_index_map)
 
@@ -32,34 +32,41 @@ Current implementation supports:
 
 This crate provides a derive macro `MultiIndexMap`, which when applied to the struct representing an element will generate a map to store and access these elements.
 Annotations are used to specify which fields to index. Currently `hashed_unique`, `hashed_non_unique`, `ordered_unique`, and `ordered_non_unique` are supported.
-The element must implement `Clone`.
+The types of all indexed fields must implement `Clone`.
+If the MultiIndexMap needs to be cloned, `Clone` must be implemented manually, see `examples/main.rs` for an example of how to do this.
 
 ## Example
 
 ```rust
 use multi_index_map::MultiIndexMap;
 
-#[derive(MultiIndexMap, Clone, Debug)]
+#[derive(MultiIndexMap, Debug)]
 struct Order {
     #[multi_index(hashed_unique)]
     order_id: u32,
     #[multi_index(ordered_unique)]
     timestamp: u64,
     #[multi_index(hashed_non_unique)]
     trader_name: String,
+    filled: bool,
+    volume: u64,
 }
 
 fn main() {
     let order1 = Order {
         order_id: 1,
         timestamp: 1656145181,
         trader_name: "JohnDoe".into(),
+        filled: false,
+        volume: 100,
     };
 
     let order2 = Order {
         order_id: 2,
         timestamp: 1656145182,
         trader_name: "JohnDoe".into(),
+        filled: false,
+        volume: 100,
     };
 
     let mut map = MultiIndexOrderMap::default();
@@ -80,10 +87,20 @@ fn main() {
             o.order_id = 42;
         })
         .unwrap();
+
     assert_eq!(order2_ref.timestamp, 1656145183);
     assert_eq!(order2_ref.order_id, 42);
     assert_eq!(order2_ref.trader_name, "JohnDoe".to_string());
 
+    let order2_ref = map
+        .update_by_order_id(&42, |filled: &mut bool, volume: &mut u64| {
+            *filled = true;
+            *volume = 0;
+        })
+        .unwrap();
+    assert_eq!(order2_ref.filled, true);
+    assert_eq!(order2_ref.volume, 0);
+
     let orders = map.get_by_trader_name(&"JohnDoe".to_string());
     assert_eq!(orders.len(), 2);
     println!("Found 2 orders for JohnDoe: [{orders:?}]");
@@ -109,10 +126,12 @@ For `hashed_unique` and `hashed_non_unique` a `FxHashMap` is used, for `ordered_
 * When retrieving elements for a given key, we lookup the key in the lookup table, then retrieve the item at that index in the backing store.
 * When removing an element for a given key, we do the same, but we then must also remove keys from all the other lookup tables before returning the element.
 * When iterating over an index, we use the default iterators for the lookup table, then simply retrieve the element at the given index in the backing store.
-* When modifying an element, we lookup the element through the given key, then apply the closure to modify the element in-place. We then return a reference to the modified element.
-We must then update all the lookup tables to account for any changes to indexed fields.
-If we only want to modify an unindexed field then it is much faster to just mutate that field directly.
-This is why the unsafe methods are provided. These can be used to modify unindexed fields quickly, but must not be used to modify indexed fields.
+* When updating un-indexed fields, we lookup the element(s) through the given key, then apply the closure to modify just the unindexed fields in-place.
+We then return a reference to the modified element(s).
+If the key doesn't match, the closure won't be applied, and Option::None will be returned.
+* When modifying indexed fields of an element, we do the same process, but the closure takes a mutable reference to the whole element.
+Any fields, indexed and un-indexed can be modified.
+We must then update all the lookup tables to account for any changes to indexed fields, so this is slower than an un-indexed update.
 
 
 ```rust
@@ -142,21 +161,21 @@ impl MultiIndexOrderMap {
     fn is_empty(&self) -> bool;
     fn clear(&mut self);
 
-    fn get_by_order_id(&self) -> Option<&Order>;
-    fn get_by_timestamp(&self) -> Option<&Order>;
-    fn get_by_trader_name(&self) -> Vec<&Order>;
-    
-    unsafe fn get_mut_by_order_id(&mut self) -> Option<&mut Order>;
-    unsafe fn get_mut_by_timestamp(&mut self) -> Option<&mut Order>;
-    unsafe fn get_mut_by_trader_name(&mut self) -> Vec<&mut Order>;
+    fn get_by_order_id(&self, key: &u32) -> Option<&Order>;
+    fn get_by_timestamp(&self, key: &u64) -> Option<&Order>;
+    fn get_by_trader_name(&self, key: &String) -> Vec<&Order>;
+
+    fn update_by_order_id(&mut self, key: &u32, f: impl FnOnce(&mut bool, &mut u64)) -> Option<&Order>;
+    fn update_by_timestamp(&mut self, key: &u64, f: impl FnOnce(&mut bool, &mut u64)) -> Option<&Order>;
+    fn update_by_trader_name(&mut self, key: &String, f: impl FnMut(&mut bool, &mut u64)) -> Vec<&Order>;
 
-    fn modify_by_order_id(&mut self, f: impl FnOnce(&mut Order)) -> Option<&Order>;
-    fn modify_by_timestamp(&mut self, f: impl FnOnce(&mut Order)) -> Option<&Order>;
-    fn modify_by_trader_name(&mut self, f: impl Fn(&mut Order)) -> Vec<&Order>;
+    fn modify_by_order_id(&mut self, key: &u32, f: impl FnOnce(&mut Order)) -> Option<&Order>;
+    fn modify_by_timestamp(&mut self, key: &u64, f: impl FnOnce(&mut Order)) -> Option<&Order>;
+    fn modify_by_trader_name(&mut self, key: &String, f: impl FnMut(&mut Order)) -> Vec<&Order>;
 
-    fn remove_by_order_id(&mut self) -> Option<Order>;
-    fn remove_by_timestamp(&mut self) -> Option<Order>;
-    fn remove_by_trader_name(&mut self) -> Vec<Order>;
+    fn remove_by_order_id(&mut self, key: &u32) -> Option<Order>;
+    fn remove_by_timestamp(&mut self, key: &u64) -> Option<Order>;
+    fn remove_by_trader_name(&mut self, key: &String) -> Vec<Order>;
 
     fn iter(&self) -> slab::Iter<Order>;
     unsafe fn iter_mut(&mut self) -> slab::IterMut<Order>;

RELEASES.md

@@ -1,3 +1,25 @@
+Version 0.8.1 (2023-08-30)
+==========================
+
+- Allow FnMut closures in `modify_by_` methods.
+
+Version 0.8.0 (2023-08-30)
+==========================
+
+- Remove `Clone` requirement on elements, now only the indexed fields must implement Clone. This should be helpful when storing non-Clonable types in un-indexed fields.
+- If the MultiIndexMap does need to be Cloned, this must be implemented manually, however this should be fairly simple to do next to where the element is defined. See `examples/main.rs`.
+
+Version 0.7.1 (2023-08-30)
+==========================
+
+- Refactor and cleanup lots of code, also further reduce work done at compile time, by only generating identifiers for each field once.
+- Implement work necessary to remove Clone requirement, however this will be fully removed in the next release.
+
+Version 0.7.0 (2023-08-29)
+==========================
+
+- Add `update_by_` methods and deprecate `get_mut_by_` methods. The new methods are equivalently useful, but safe and equally performant.
+
 Version 0.6.2 (2023-08-15)
 ==========================
 

multi_index_map/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "multi_index_map"
-version = "0.6.2"
+version = "0.8.1"
 edition = "2021"
 authors = ["Louis Wyborn <louiswyborn@gmail.com>"]
 rust-version = "1.62"
@@ -14,7 +14,7 @@ readme = "README.md"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-multi_index_map_derive = { version = "0.6.2", path = "../multi_index_map_derive" }
+multi_index_map_derive = { version = "0.8.1", path = "../multi_index_map_derive" }
 
 # Used as the backing store of all the elements.
 slab = { version = "0.4" }
@@ -31,4 +31,4 @@ name = "performance"
 harness = false
 
 [features]
-experimental = ["multi_index_map_derive/experimental"]
+trivial_bounds = ["multi_index_map_derive/trivial_bounds"]

multi_index_map/benches/performance.rs

@@ -16,6 +16,18 @@ pub struct TestElementWithOnlyIndexedFields {
     field_ordered_non_unique: u32,
 }
 
+impl Clone for MultiIndexTestElementWithOnlyIndexedFieldsMap {
+    fn clone(&self) -> Self {
+        Self {
+            _store: self._store.clone(),
+            _field_hashed_unique_index: self._field_hashed_unique_index.clone(),
+            _field_hashed_non_unique_index: self._field_hashed_non_unique_index.clone(),
+            _field_ordered_unique_index: self._field_ordered_unique_index.clone(),
+            _field_ordered_non_unique_index: self._field_ordered_non_unique_index.clone(),
+        }
+    }
+}
+
 const BENCH_SIZES: &[u32] = &[100u32, 1000u32, 10000u32, 100000u32];
 
 fn insert_benchmark(c: &mut Criterion) {

multi_index_map/examples/main.rs

@@ -13,6 +13,18 @@ mod inner {
         pub(crate) trader_name: String,
         pub(crate) note: String,
     }
+
+    // Manually implement Clone, this can be auto generated correctly by rust-analyzer
+    impl Clone for MultiIndexOrderMap {
+        fn clone(&self) -> Self {
+            Self {
+                _store: self._store.clone(),
+                _order_id_index: self._order_id_index.clone(),
+                _timestamp_index: self._timestamp_index.clone(),
+                _trader_name_index: self._trader_name_index.clone(),
+            }
+        }
+    }
 }
 
 fn main() {
@@ -75,12 +87,12 @@ fn main() {
         o1_ref.trader_name, o1_ref
     );
 
-    let o1_mut_ref = unsafe { map.get_mut_by_order_id(&7).unwrap() };
-    o1_mut_ref.note = "TestNote".to_string();
-    println!(
-        "Changed note of order {o1_mut_ref:?}, to {:?}",
-        o1_mut_ref.note,
-    );
+    let o1_ref = map
+        .update_by_order_id(&7, |note| {
+            *note = "TestNote".to_string();
+        })
+        .unwrap();
+    println!("Updated note of order {o1_ref:?}, to {:?}", o1_ref.note,);
 
     let toms_orders = map.remove_by_trader_name(&"Tom".to_string());
     assert_eq!(toms_orders.len(), 2);

multi_index_map/tests/capacity_manipulations.rs

@@ -1,6 +1,6 @@
 use multi_index_map::MultiIndexMap;
 
-#[derive(MultiIndexMap, Clone, Debug)]
+#[derive(MultiIndexMap, Debug)]
 struct TestElement {
     #[multi_index(hashed_unique)]
     field1: i32,

multi_index_map/tests/debug.rs

@@ -0,0 +1,33 @@
+#![cfg_attr(feature = "trivial_bounds", feature(trivial_bounds))]
+#![cfg(feature = "trivial_bounds")]
+use multi_index_map::MultiIndexMap;
+
+#[derive(Hash, PartialEq, Eq, Clone, Debug)]
+struct TestNonPrimitiveType(u64);
+
+#[derive(MultiIndexMap, Clone, Debug)]
+struct TestElement {
+    #[multi_index(hashed_unique)]
+    field1: TestNonPrimitiveType,
+    field2: String,
+}
+
+#[test]
+fn should_compile() {
+    let mut map = MultiIndexTestElementMap::default();
+
+    // check that formatting produces non empty strings
+    assert!(!format!("{:?}", map._field1_index).is_empty());
+    assert!(!format!("{:?}", map._store).is_empty());
+    assert!(!format!("{:?}", map).is_empty());
+
+    let elem1 = TestElement {
+        field1: TestNonPrimitiveType(42),
+        field2: "ElementOne".to_string(),
+    };
+    map.insert(elem1);
+
+    let msg = format!("{:?}", map);
+    // check if stored field 1 is present in debug output
+    assert!(msg.contains("42"));
+}

multi_index_map/tests/get_and_modify_mut.rs

@@ -1,9 +1,11 @@
+#![cfg_attr(feature = "trivial_bounds", feature(trivial_bounds))]
+
 use multi_index_map::MultiIndexMap;
 
 #[derive(Hash, PartialEq, Eq, Clone)]
 struct TestNonPrimitiveType(u64);
 
-#[derive(MultiIndexMap, Clone)]
+#[derive(MultiIndexMap)]
 struct TestElement {
     #[multi_index(hashed_non_unique)]
     field1: usize,

multi_index_map/tests/hashed_non_unique.rs

@@ -3,7 +3,7 @@ use multi_index_map::MultiIndexMap;
 #[derive(Hash, PartialEq, Eq, Clone, Debug)]
 struct TestNonPrimitiveType(u64);
 
-#[derive(MultiIndexMap, Clone, Debug)]
+#[derive(MultiIndexMap, Debug)]
 struct TestElement {
     #[multi_index(hashed_non_unique)]
     field1: TestNonPrimitiveType,

multi_index_map/tests/hashed_unique.rs

@@ -1,9 +1,11 @@
+#![cfg_attr(feature = "trivial_bounds", feature(trivial_bounds))]
+
 use multi_index_map::MultiIndexMap;
 
 #[derive(Hash, PartialEq, Eq, Clone)]
 struct TestNonPrimitiveType(u64);
 
-#[derive(MultiIndexMap, Clone)]
+#[derive(MultiIndexMap)]
 struct TestElement {
     #[multi_index(hashed_unique)]
     field1: TestNonPrimitiveType,

multi_index_map/tests/iter_after_modify.rs

@@ -1,6 +1,6 @@
 use multi_index_map::MultiIndexMap;
 
-#[derive(MultiIndexMap, Debug, Clone)]
+#[derive(MultiIndexMap, Debug)]
 pub(crate) struct Order {
     #[multi_index(hashed_unique)]
     pub(crate) order_id: u32,
@@ -51,10 +51,15 @@ fn iter_after_modify() {
         assert_eq!(it.next().unwrap().order_id, 1);
     }
 
+    let mut s = "test".to_string();
+
     map.modify_by_order_id(&1, |o| {
-        o.timestamp = 0;
+        s = "p".to_string();
+        o.timestamp = 4;
     });
 
+    assert_eq!(s, "p");
+
     {
         let mut it = map.iter_by_timestamp();
         assert_eq!(it.next().unwrap().order_id, 1);

multi_index_map/tests/mixed_non_unique.rs

@@ -1,6 +1,8 @@
+#![cfg_attr(feature = "trivial_bounds", feature(trivial_bounds))]
+
 use multi_index_map::MultiIndexMap;
 
-#[derive(MultiIndexMap, Clone)]
+#[derive(MultiIndexMap)]
 struct MultipleOrderedNonUniqueStruct {
     #[multi_index(ordered_non_unique)]
     field1: u32,

multi_index_map/tests/ordered_non_unique.rs

@@ -3,7 +3,7 @@ use multi_index_map::MultiIndexMap;
 #[derive(Hash, PartialEq, Eq, Clone, Debug, PartialOrd, Ord)]
 struct TestNonPrimitiveType(u64);
 
-#[derive(MultiIndexMap, Clone, Debug)]
+#[derive(MultiIndexMap, Debug)]
 struct TestElement {
     #[multi_index(ordered_non_unique)]
     field1: TestNonPrimitiveType,

multi_index_map/tests/ordered_unique.rs

@@ -1,9 +1,11 @@
+#![cfg_attr(feature = "trivial_bounds", feature(trivial_bounds))]
+
 use multi_index_map::MultiIndexMap;
 
 #[derive(Hash, PartialEq, Eq, Clone, PartialOrd, Ord)]
 struct TestNonPrimitiveType(u64);
 
-#[derive(MultiIndexMap, Clone)]
+#[derive(MultiIndexMap)]
 struct TestElement {
     #[multi_index(ordered_unique)]
     field1: TestNonPrimitiveType,

multi_index_map/tests/reverse_iter.rs

@@ -1,5 +1,5 @@
 use multi_index_map::MultiIndexMap;
-#[derive(MultiIndexMap, Clone, PartialEq, Debug)]
+#[derive(MultiIndexMap, PartialEq, Debug)]
 struct TestElement {
     #[multi_index(ordered_non_unique)]
     field1: usize,