From 8c7e29a13cfc1daba326b0ae3e6727b31e894f94 Mon Sep 17 00:00:00 2001 From: JCW Date: Mon, 8 Jun 2026 15:32:13 +0100 Subject: [PATCH] Fix issues --- include/xrpl/basics/TaggedCache.h | 81 +++++++++++++++--------- include/xrpl/basics/TaggedCache.ipp | 94 ++++++++++++++++++++++++---- src/test/basics/TaggedCache_test.cpp | 2 +- 3 files changed, 132 insertions(+), 45 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 5941b6b7dd..7ea939c47d 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -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 - using CanonicalizeClientPointerType = std::conditional_t< - std::is_same_v, - SharedPointerType const&, - SharedPointerType&>; + // writable. Defined in TaggedCache.ipp. + template + 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 + bool + canonicalizeImpl( + key_type const& key, + typename CanonicalizeClientPointerType::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 + template bool - canonicalize(key_type const& key, CanonicalizeClientPointerType 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); diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index dce18f9a14..c813374e85 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/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 +struct TaggedCache< + Key, + T, + IsKeyCache, + SharedWeakUnionPointer, + SharedPointerType, + Hash, + KeyEqual, + Mutex>::CanonicalizeClientPointerType +{ + using Type = std::conditional_t< + std::is_same_v, + SharedPointerType const&, + SharedPointerType&>; +}; + template < class Key, class T, @@ -300,28 +350,28 @@ template < class Hash, class KeyEqual, class Mutex> -template +template inline bool TaggedCache:: - canonicalize( + canonicalizeImpl( key_type const& key, - CanonicalizeClientPointerType data, - [[maybe_unused]] R&& replacePolicy) + typename CanonicalizeClientPointerType::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; - constexpr bool replaceCached = std::is_same_v; + constexpr bool replaceCached = std::is_same_v; std::scoped_lock const lock(mutex_); @@ -345,13 +395,13 @@ TaggedCache) + else if constexpr (std::is_same_v) { return false; } else { - return replacePolicy(entry.ptr.getStrong()); + return replaceCallback(entry.ptr.getStrong()); } }; @@ -393,6 +443,24 @@ TaggedCache +template +inline bool +TaggedCache:: + canonicalize(key_type const& key, SharedPointerType& data, Callback&& replaceCallback) +{ + return canonicalizeImpl( + key, data, detail::ReplaceDynamically{}, std::forward(replaceCallback)); +} + template < class Key, class T, @@ -406,7 +474,7 @@ inline bool TaggedCache:: 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:: canonicalizeReplaceClient(key_type const& key, SharedPointerType& data) { - return canonicalize(key, data, detail::ReplaceClient{}); + return canonicalizeImpl(key, data, detail::ReplaceClient{}); } template < diff --git a/src/test/basics/TaggedCache_test.cpp b/src/test/basics/TaggedCache_test.cpp index 5de18b7870..20e7879a36 100644 --- a/src/test/basics/TaggedCache_test.cpp +++ b/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; } };