Fix table and table_transient update()

arximboldi · Jul 5, 2022 · f3cebc6 · f3cebc6
1 parent bd1f0c0
commit f3cebc6
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 30 deletions.
diff --git a/immer/table.hpp b/immer/table.hpp
@@ -21,21 +21,38 @@ class table_transient;
  * It assumes the key is `id` class member.
  */
 template <typename T>
-auto get_table_key(const T& x) -> decltype(x.id)
+auto get_table_key(T const& x) -> decltype(x.id)
 {
     return x.id;
 }
 
+/*!
+ * Function template to set the key in `immer::table_key_fn`.
+ * It assumes the key is `id` class member.
+ */
+template <typename T, typename K>
+auto set_table_key(T x, K&& k) -> T
+{
+    x.id = std::forward<K>(k);
+    return x;
+}
+
 /*!
  * Default value for `KeyFn` in `immer::table`.
  * It assumes the key is `id` class member.
  */
 struct table_key_fn
 {
     template <typename T>
-    decltype(auto) operator()(const T& x) const
+    decltype(auto) operator()(T&& x) const
     {
-        return get_table_key(x);
+        return get_table_key(std::forward<T>(x));
+    }
+
+    template <typename T, typename K>
+    auto operator()(T&& x, K&& k) const
+    {
+        return set_table_key(std::forward<T>(x), std::forward<K>(k));
     }
 };
 
@@ -108,9 +125,9 @@ class table
     struct combine_value
     {
         template <typename Kf, typename Tf>
-        value_t operator()(Kf&& k, Tf&& v) const
+        auto operator()(Kf&& k, Tf&& v) const
         {
-            return std::forward<Tf>(v);
+            return KeyFn{}(std::forward<Tf>(v), std::forward<Kf>(k));
         }
     };
 
@@ -133,33 +150,39 @@ class table
 
     struct hash_key
     {
-        auto operator()(const value_t& v) { return Hash{}(KeyFn{}(v)); }
+        std::size_t operator()(const value_t& v) const
+        {
+            return Hash{}(KeyFn{}(v));
+        }
 
         template <typename Key>
-        auto operator()(const Key& v)
+        std::size_t operator()(const Key& v) const
         {
             return Hash{}(v);
         }
     };
 
     struct equal_key
     {
-        auto operator()(const value_t& a, const value_t& b)
+        bool operator()(const value_t& a, const value_t& b) const
         {
             auto ke = KeyFn{};
             return Equal{}(ke(a), ke(b));
         }
 
         template <typename Key>
-        auto operator()(const value_t& a, const Key& b)
+        bool operator()(const value_t& a, const Key& b) const
         {
             return Equal{}(KeyFn{}(a), b);
         }
     };
 
     struct equal_value
     {
-        auto operator()(const value_t& a, const value_t& b) { return a == b; }
+        bool operator()(const value_t& a, const value_t& b) const
+        {
+            return a == b;
+        }
     };
 
     using impl_t =
@@ -386,8 +409,9 @@ class table
 
     /*!
      * Returns `this->insert(fn((*this)[k]))`. In particular, `fn` maps
-     * `T` to `T`. The `fn` return value should have key `k`.
-     * It may allocate memory and its complexity is *effectively* @f$ O(1) @f$.
+     * `T` to `T`. The key `k` will be replaced inside the value returned by
+     * `fn`. It may allocate memory and its complexity is *effectively* @f$ O(1)
+     * @f$.
      */
     template <typename Fn>
     IMMER_NODISCARD table update(key_type k, Fn&& fn) const&

diff --git a/immer/table_transient.hpp b/immer/table_transient.hpp
@@ -213,16 +213,18 @@ class table_transient : MemoryPolicy::transience_t::owner
     void insert(value_type value) { impl_.add_mut(*this, std::move(value)); }
 
     /*!
-     * Returns `this->insert(fn((*this)[k]))`. In particular, `fn` maps
-     * `T` to `T`. The `fn` return value should have key `k`.
-     * It may allocate memory and its complexity is *effectively* @f$ O(1) @f$.
+     * Returns `this->insert(fn((*this)[k]))`. In particular, `fn` maps `T` to
+     * `T`. The key `k` will be set into the value returned bu `fn`.  It may
+     * allocate memory and its complexity is *effectively* @f$ O(1) @f$.
      */
     template <typename Fn>
     void update(key_type k, Fn&& fn)
     {
-        impl_ = impl_.template update<persistent_type::project_value,
-                                      persistent_type::default_value,
-                                      persistent_type::combine_value>(
+        impl_.template update_mut<typename persistent_type::project_value,
+                                  typename persistent_type::default_value,
+                                  typename persistent_type::combine_value>(
+            *this, std::move(k), std::forward<Fn>(fn));
+    }
             *this, std::move(k), std::forward<Fn>(fn));
     }
 

diff --git a/test/table/generic.ipp b/test/table/generic.ipp
@@ -31,11 +31,25 @@ struct pair_key_fn
         return p.first;
     }
 
+    template <typename F, typename S>
+    auto operator()(std::pair<F, S> p, F k) const
+    {
+        p.first = std::move(k);
+        return p;
+    }
+
     template <typename F, typename S>
     F operator()(const dadaist<std::pair<F, S>>& p) const
     {
         return p.value.first;
     }
+
+    template <typename F, typename S>
+    auto operator()(dadaist<std::pair<F, S>> p, F k) const
+    {
+        p.value.first = std::move(k);
+        return p;
+    }
 };
 
 template <typename K,
@@ -300,8 +314,7 @@ TEST_CASE("exception safety")
                     return make_pair(x.value.first, x.value.second + 1);
                 });
                 ++i;
-            } catch (dada_error) {
-            }
+            } catch (dada_error) {}
             for (auto i : test_irange(0u, i))
                 CHECK(v.at(i).value.second == i + 1);
             for (auto i : test_irange(i, n))
@@ -333,8 +346,7 @@ TEST_CASE("exception safety")
                     return make_pair(x.value.first, x.value.second + 1);
                 });
                 ++i;
-            } catch (dada_error) {
-            }
+            } catch (dada_error) {}
             for (auto i : test_irange(0u, i))
                 CHECK(v.at(vals[i].first).value.second == vals[i].second + 1);
             for (auto i : test_irange(i, n))
@@ -358,8 +370,7 @@ TEST_CASE("exception safety")
                     return make_pair(x.value.first, x.value.second + 1);
                 });
                 ++i;
-            } catch (dada_error) {
-            }
+            } catch (dada_error) {}
             for (auto i : test_irange(0u, i))
                 CHECK(v.at(vals[i].first).value.second == vals[i].second + 1);
             for (auto i : test_irange(i, n))
@@ -376,17 +387,15 @@ struct KeyType
 {
     explicit KeyType(uint32_t v)
         : value(v)
-    {
-    }
+    {}
     uint32_t value;
 };
 
 struct LookupType
 {
     explicit LookupType(uint32_t v)
         : value(v)
-    {
-    }
+    {}
     uint32_t value;
 };
 

diff --git a/test/table_transient/generic.ipp b/test/table_transient/generic.ipp
@@ -16,8 +16,8 @@
 
 struct Item
 {
-    std::string id;
-    int value;
+    std::string id{};
+    int value{};
 
     bool operator==(const Item& other) const
     {
@@ -62,6 +62,20 @@ TEST_CASE("insert")
     t.insert(Item{"foo", 6});
     CHECK(t["foo"].value == 6);
     CHECK(t.size() == 2);
+
+    t.update("foo", [](auto item) {
+        item.value += 1;
+        return item;
+    });
+    CHECK(t["foo"].value == 7);
+    CHECK(t.size() == 2);
+
+    t.update("lol", [](auto item) {
+        item.value += 1;
+        return item;
+    });
+    CHECK(t["lol"].value == 1);
+    CHECK(t.size() == 3);
 }
 
 TEST_CASE("erase")