From c412fc70bffc6cd8b521100279e41a0aea0f498a Mon Sep 17 00:00:00 2001 From: LTLA Date: Mon, 8 Apr 2024 09:55:16 -0700 Subject: [PATCH] Delete copy construction/assignment for cache classes. This is necessary as the cache classes hold iterators/pointers to their own internal members; any copy would have incorrect references to the original, and I can't be bothered to go through and update them on copy. Move construction/assignment is still okay, as the targets of the iterators and pointers are moved into the new object. --- include/tatami_chunked/LruSlabCache.hpp | 31 ++++++++++++++++++- include/tatami_chunked/OracleSlabCache.hpp | 16 ++++++++-- .../SubsettedOracleSlabCache.hpp | 16 ++++++++-- include/tatami_chunked/typical_slab_cache.hpp | 31 +++++++++---------- 4 files changed, 73 insertions(+), 21 deletions(-) diff --git a/include/tatami_chunked/LruSlabCache.hpp b/include/tatami_chunked/LruSlabCache.hpp index f5ec6cc..b45ca26 100644 --- a/include/tatami_chunked/LruSlabCache.hpp +++ b/include/tatami_chunked/LruSlabCache.hpp @@ -36,7 +36,36 @@ class LruSlabCache { /** * @param m Maximum number of slabs to store. */ - LruSlabCache(size_t m = 1) : max_slabs(m) {} + LruSlabCache(size_t m) : max_slabs(m) {} + + /** + * Deleted as the cache holds persistent iterators. + */ + LruSlabCache(const LruSlabCache&) = delete; + + /** + * Deleted as the cache holds persistent iterators. + */ + LruSlabCache& operator=(const LruSlabCache&) = delete; + + /** + * @cond + */ + // Iterators are guaranteed to be valid after move, see Notes in + // https://en.cppreference.com/w/cpp/container/list/list + // https://en.cppreference.com/w/cpp/container/list/operator%3D + LruSlabCache& operator=(LruSlabCache&&) = default; + LruSlabCache(LruSlabCache&&) = default; + /** + * @endcond + */ + + /** + * Overloaded constructor for consistency with `OracleSlabCache` and `SubsettedOracleSlabCache`. + * @tparam Oracle_ Any class, to be ignored. + */ + template + LruSlabCache([[maybe_unused]] Oracle_ ora, size_t m) : LruSlabCache(m) {} public: /** diff --git a/include/tatami_chunked/OracleSlabCache.hpp b/include/tatami_chunked/OracleSlabCache.hpp index 6154231..b1380ea 100644 --- a/include/tatami_chunked/OracleSlabCache.hpp +++ b/include/tatami_chunked/OracleSlabCache.hpp @@ -57,11 +57,23 @@ class OracleSlabCache { future_cache.reserve(max_slabs); } + + /** + * Deleted as the cache holds persistent pointers. + */ + OracleSlabCache(const OracleSlabCache&) = delete; + + /** + * Deleted as the cache holds persistent pointers. + */ + OracleSlabCache& operator=(const OracleSlabCache&) = delete; + /** * @cond */ - // For testing only. - OracleSlabCache() = default; + // Move operators are still okay. + OracleSlabCache& operator=(OracleSlabCache&&) = default; + OracleSlabCache(OracleSlabCache&&) = default; /** * @endcond */ diff --git a/include/tatami_chunked/SubsettedOracleSlabCache.hpp b/include/tatami_chunked/SubsettedOracleSlabCache.hpp index 25db834..2112657 100644 --- a/include/tatami_chunked/SubsettedOracleSlabCache.hpp +++ b/include/tatami_chunked/SubsettedOracleSlabCache.hpp @@ -190,11 +190,23 @@ class SubsettedOracleSlabCache { } } + /** + * Deleted as the cache holds persistent pointers. + */ + SubsettedOracleSlabCache(const SubsettedOracleSlabCache&) = delete; + + /** + * Deleted as the cache holds persistent pointers. + */ + SubsettedOracleSlabCache& operator=(const SubsettedOracleSlabCache&) = delete; + /** * @cond */ - // For testing only. - SubsettedOracleSlabCache() = default; + // Move operators are still okay as pointers still point to the moved vectors, + // see https://stackoverflow.com/questions/43988553/stdvector-stdmove-and-pointer-invalidation. + SubsettedOracleSlabCache(SubsettedOracleSlabCache&&) = delete; + SubsettedOracleSlabCache& operator=(SubsettedOracleSlabCache&&) = delete; /** * @endcond */ diff --git a/include/tatami_chunked/typical_slab_cache.hpp b/include/tatami_chunked/typical_slab_cache.hpp index 84eb578..52e2e46 100644 --- a/include/tatami_chunked/typical_slab_cache.hpp +++ b/include/tatami_chunked/typical_slab_cache.hpp @@ -46,25 +46,24 @@ struct TypicalSlabCacheWorkspace { * @param oracle Oracle containing the predicted accesses. * Only relevant if `oracle_ = true`, otherwise a bool should be passed in (which is ignored). */ - TypicalSlabCacheWorkspace(Index_ primary_length, Index_ secondary_length, size_t cache_size_in_elements, bool require_minimum_cache, tatami::MaybeOracle oracle) { - slab_size_in_elements = static_cast(primary_length) * static_cast(secondary_length); + TypicalSlabCacheWorkspace(Index_ primary_length, Index_ secondary_length, size_t cache_size_in_elements, bool require_minimum_cache, tatami::MaybeOracle oracle) : + slab_size_in_elements(static_cast(primary_length) * static_cast(secondary_length)), + num_slabs_in_cache(compute_num_slabs_in_cache(slab_size_in_elements, cache_size_in_elements, require_minimum_cache)), + cache(std::move(oracle), num_slabs_in_cache) + {} - if (!slab_size_in_elements) { - num_slabs_in_cache = 0; - } else { - num_slabs_in_cache = (slab_size_in_elements ? cache_size_in_elements / slab_size_in_elements : 1); - if (num_slabs_in_cache == 0 && require_minimum_cache) { - num_slabs_in_cache = 1; - } +private: + static size_t compute_num_slabs_in_cache(size_t slab_size_in_elements, size_t cache_size_in_elements, bool require_minimum_cache) { + if (slab_size_in_elements == 0) { + return 0; } - if constexpr(!oracle_) { - cache = LruSlabCache(num_slabs_in_cache); - } else if constexpr(!subset_) { - cache = OracleSlabCache(std::move(oracle), num_slabs_in_cache); - } else { - cache = SubsettedOracleSlabCache(std::move(oracle), num_slabs_in_cache); - } + auto tmp = cache_size_in_elements / slab_size_in_elements; + if (tmp == 0 && require_minimum_cache) { + return 1; + } + + return tmp; } public: