Fix issues

include/xrpl/basics/TaggedCache.h

@@ -9,6 +9,7 @@
 #include <xrpl/beast/insight/Insight.h>
 
 #include <atomic>
+#include <cstddef>
 #include <functional>
 #include <mutex>
 #include <thread>
@@ -17,23 +18,6 @@
 
 namespace xrpl {
 
-namespace detail {
-
-// Replace-policy tags used by TaggedCache::canonicalizeReplaceCache /
-// canonicalizeReplaceClient when calling canonicalize. With ReplaceCached the
-// cached value is always replaced with `data`, so `data` is never written back
-// and may be const. With ReplaceClient the cached value is kept and written
-// back into `data` (the client's pointer), which must therefore be writable.
-struct ReplaceCached
-{
-};
-
-struct ReplaceClient
-{
-};
-
-}  // namespace detail
-
 /** Map/cache combination.
     This class implements a cache and a map. The cache keeps objects alive
     in the map. The map allows multiple code paths that reference objects
@@ -114,14 +98,27 @@ public:
     del(key_type const& key, bool valid);
 
 private:
-    // Selects the `data` parameter type of canonicalize from the replace
+    // Selects the `data` parameter type of canonicalizeImpl from the replace
     // policy: const for detail::ReplaceCached (never written back), otherwise
-    // writable.
-    template <typename R>
-    using CanonicalizeClientPointerType = std::conditional_t<
-        std::is_same_v<detail::ReplaceCached, R>,
-        SharedPointerType const&,
-        SharedPointerType&>;
+    // writable. Defined in TaggedCache.ipp.
+    template <typename Policy>
+    struct CanonicalizeClientPointerType;
+
+    /** Shared implementation of the canonicalize family.
+
+        `policy` selects how a collision is resolved when `key` already exists:
+        detail::ReplaceCached, detail::ReplaceClient or
+        detail::ReplaceDynamically. For ReplaceDynamically `replaceCallback` is
+        invoked with the existing strong pointer and returns whether to replace
+        the cached value with `data`; for the tag policies it is unused.
+    */
+    template <class Policy, class Callback = std::nullptr_t>
+    bool
+    canonicalizeImpl(
+        key_type const& key,
+        typename CanonicalizeClientPointerType<Policy>::Type data,
+        Policy policy,
+        Callback&& replaceCallback = nullptr);
 
 public:
     /** Replace aliased objects with originals.
@@ -131,27 +128,49 @@ public:
         This routine eliminates the duplicate and performs a replacement
         on the callers shared pointer if needed.
 
-        `replacePolicy` is a callable taking the existing strong pointer and
+        `replaceCallback` is a callable taking the existing strong pointer and
         returning whether to replace the cached value with `data` (true) or to
         keep the cached value and write it back into `data` (false). Because the
         write-back case mutates `data`, `data` must be writable.
 
-        (canonicalizeReplaceCache / canonicalizeReplaceClient call this with
-        internal replace-policy tags in place of a callable.)
-
         @param key The key corresponding to the object
         @param data A shared pointer to the data corresponding to the object.
-        @param replacePolicy A callable (existing strong pointer -> bool).
+        @param replaceCallback A callable (existing strong pointer -> bool).
 
         @return `true` If the key already existed.
     */
-    template <class R>
+    template <class Callback>
     bool
-    canonicalize(key_type const& key, CanonicalizeClientPointerType<R> data, R&& replacePolicy);
+    canonicalize(key_type const& key, SharedPointerType& data, Callback&& replaceCallback);
 
+    /** Insert/update the canonical entry for `key`, always replacing the
+        cached value with `data`.
+
+        If an entry already exists for `key`, the cached value is unconditionally
+        replaced with `data`; otherwise `data` is inserted. `data` is never
+        written back, so it may be const.
+
+        @param key The key corresponding to the object.
+        @param data A shared pointer to the data corresponding to the object.
+
+        @return `true` If the key already existed.
+    */
     bool
     canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data);
 
+    /** Insert the canonical entry for `key`, keeping any existing cached value.
+
+        If an entry already exists for `key`, the cached value is kept and
+        written back into `data` so the caller ends up with the canonical
+        object; otherwise `data` is inserted. Because `data` may be overwritten
+        it must be writable.
+
+        @param key The key corresponding to the object.
+        @param data A shared pointer to the data corresponding to the object;
+                    updated to the canonical value when one already exists.
+
+        @return `true` If the key already existed.
+    */
     bool
     canonicalizeReplaceClient(key_type const& key, SharedPointerType& data);
 

include/xrpl/basics/TaggedCache.ipp

@@ -5,6 +5,56 @@
 
 namespace xrpl {
 
+namespace detail {
+
+// Replace-policy tags selecting how TaggedCache::canonicalizeImpl resolves a
+// collision when the key already exists:
+//   - ReplaceCached: always replace the cached value with `data`. `data` is
+//     never written back and may be const.
+//   - ReplaceClient: keep the cached value and write it back into `data` (the
+//     client's pointer), which must therefore be writable.
+//   - ReplaceDynamically: call the supplied callback to decide per call; `data`
+//     is written back when the cached value is kept, so it must be writable.
+struct ReplaceCached
+{
+};
+
+struct ReplaceClient
+{
+};
+
+struct ReplaceDynamically
+{
+};
+
+}  // namespace detail
+
+template <
+    class Key,
+    class T,
+    bool IsKeyCache,
+    class SharedWeakUnionPointer,
+    class SharedPointerType,
+    class Hash,
+    class KeyEqual,
+    class Mutex>
+template <typename Policy>
+struct TaggedCache<
+    Key,
+    T,
+    IsKeyCache,
+    SharedWeakUnionPointer,
+    SharedPointerType,
+    Hash,
+    KeyEqual,
+    Mutex>::CanonicalizeClientPointerType
+{
+    using Type = std::conditional_t<
+        std::is_same_v<detail::ReplaceCached, Policy>,
+        SharedPointerType const&,
+        SharedPointerType&>;
+};
+
 template <
     class Key,
     class T,
@@ -300,28 +350,28 @@ template <
     class Hash,
     class KeyEqual,
     class Mutex>
-template <class R>
+template <class Policy, class Callback>
 inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
-    canonicalize(
+    canonicalizeImpl(
         key_type const& key,
-        CanonicalizeClientPointerType<R> data,
-        [[maybe_unused]] R&& replacePolicy)
+        typename CanonicalizeClientPointerType<Policy>::Type data,
+        [[maybe_unused]] Policy policy,
+        [[maybe_unused]] Callback&& replaceCallback)
 {
     // Return canonical value, store if needed, refresh in cache
     // Return values: true=we had the data already
 
-    // `replacePolicy` is one of:
+    // `Policy` is one of:
     //   - detail::ReplaceCached: always replace the cached value with `data`;
     //     `data` is never written back and may be const.
     //   - detail::ReplaceClient: keep the cached value and write it back into
     //     `data` (the client's pointer), which must therefore be writable.
-    //   - a callable(strong pointer) -> bool: decide at run time; `data` must
-    //     be writable.
+    //   - detail::ReplaceDynamically: call `replaceCallback` to decide at run
+    //     time; `data` must be writable.
     // For the latter two the write-back below requires a mutable `data`, so
     // passing a const argument is a compile error.
-    using C = std::remove_cvref_t<R>;
-    constexpr bool replaceCached = std::is_same_v<C, detail::ReplaceCached>;
+    constexpr bool replaceCached = std::is_same_v<Policy, detail::ReplaceCached>;
 
     std::scoped_lock const lock(mutex_);
 
@@ -345,13 +395,13 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
         {
             return true;
         }
-        else if constexpr (std::is_same_v<C, detail::ReplaceClient>)
+        else if constexpr (std::is_same_v<Policy, detail::ReplaceClient>)
         {
             return false;
         }
         else
         {
-            return replacePolicy(entry.ptr.getStrong());
+            return replaceCallback(entry.ptr.getStrong());
         }
     };
 
@@ -393,6 +443,24 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
     return false;
 }
 
+template <
+    class Key,
+    class T,
+    bool IsKeyCache,
+    class SharedWeakUnionPointer,
+    class SharedPointerType,
+    class Hash,
+    class KeyEqual,
+    class Mutex>
+template <class Callback>
+inline bool
+TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
+    canonicalize(key_type const& key, SharedPointerType& data, Callback&& replaceCallback)
+{
+    return canonicalizeImpl(
+        key, data, detail::ReplaceDynamically{}, std::forward<Callback>(replaceCallback));
+}
+
 template <
     class Key,
     class T,
@@ -406,7 +474,7 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data)
 {
-    return canonicalize(key, data, detail::ReplaceCached{});
+    return canonicalizeImpl(key, data, detail::ReplaceCached{});
 }
 
 template <
@@ -422,7 +490,7 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalizeReplaceClient(key_type const& key, SharedPointerType& data)
 {
-    return canonicalize(key, data, detail::ReplaceClient{});
+    return canonicalizeImpl(key, data, detail::ReplaceClient{});
 }
 
 template <

src/test/basics/TaggedCache_test.cpp

@@ -206,7 +206,7 @@ public:
                 bool
                 operator==(std::string const& other) const
                 {
-                    return data == other.data;
+                    return data == other;
                 }
             };