From 822e54befb9e6a49bfe884411c8f938af9b832c8 Mon Sep 17 00:00:00 2001 From: Sylvester Joosten Date: Thu, 2 Nov 2023 11:55:01 +0200 Subject: [PATCH] Correct clone behavior for algorithm mixins - Deep copy of Properties - No copy for Resources (force re-inializiation to properly relocate the internal Resource pointers) --- JugAlgo/JugAlgo/Algorithm.h | 2 -- .../algorithms/calorimetry/ClusterRecoCoG.h | 3 +-- .../core/include/algorithms/algorithm.h | 16 ++++++++++-- .../core/include/algorithms/property.h | 25 ++++++++++++------- .../core/include/algorithms/resource.h | 6 +++-- 5 files changed, 35 insertions(+), 17 deletions(-) diff --git a/JugAlgo/JugAlgo/Algorithm.h b/JugAlgo/JugAlgo/Algorithm.h index f2bd1d6f..b64023b2 100644 --- a/JugAlgo/JugAlgo/Algorithm.h +++ b/JugAlgo/JugAlgo/Algorithm.h @@ -88,8 +88,6 @@ template class Algorithm : public GaudiAlgorithm { // need to get info on MC-ness of this sample somehow TODO c.mc(true); m_algo.executeInContext(m_input.get(), m_output.get(), c); - //m_algo.context(c); - //m_algo.execute(m_input.get(), m_output.get()); } catch (const std::exception& e) { error() << e.what() << endmsg; return StatusCode::FAILURE; diff --git a/external/algorithms/calorimetry/include/algorithms/calorimetry/ClusterRecoCoG.h b/external/algorithms/calorimetry/include/algorithms/calorimetry/ClusterRecoCoG.h index 29753efb..e5a363e0 100644 --- a/external/algorithms/calorimetry/include/algorithms/calorimetry/ClusterRecoCoG.h +++ b/external/algorithms/calorimetry/include/algorithms/calorimetry/ClusterRecoCoG.h @@ -35,7 +35,7 @@ class ClusterRecoCoG : public ClusteringAlgorithm { public: using WeightFunc = std::function; - // TODO: get rid of "Collection" in names + // FIXME: get rid of "Collection" in names ClusterRecoCoG(std::string_view name) : ClusteringAlgorithm{name, {"inputProtoClusterCollection", "mcHits"}, @@ -43,7 +43,6 @@ class ClusterRecoCoG : public ClusteringAlgorithm { "Reconstruct a cluster with the Center of Gravity method. For " "simulation results it optionally creates a Cluster <-> MCParticle " "association provided both optional arguments are provided."} {} - private: void init() final; void process(const Input&, const Output&) const final; diff --git a/external/algorithms/core/include/algorithms/algorithm.h b/external/algorithms/core/include/algorithms/algorithm.h index 9ab724a4..aecfaa49 100644 --- a/external/algorithms/core/include/algorithms/algorithm.h +++ b/external/algorithms/core/include/algorithms/algorithm.h @@ -96,7 +96,14 @@ template class Algorithm : public AlgorithmH copy->context(c, copy.get()); copy->execute(i, o); } - auto clone() const { return std::make_unique(*this); } + auto clone() const { + auto copy = std::make_unique(*this); + // copy over property values + for (const auto& [key, prop] : getProperties()) { + std::visit([&](auto&& val) { copy->setProperty(key, val); }, prop.get()); + } + return copy; + } const InputNames& inputNames() const { return m_input_names; } const OutputNames& outputNames() const { return m_output_names; } @@ -105,6 +112,12 @@ template class Algorithm : public AlgorithmH virtual void init() {} virtual void process(const Input&, const Output&) const {} + // Private copy constructor as a valid copy can only be obtained through the clone method + Algorithm(const Algorithm& rhs) + : AlgorithmHandle(rhs) + , m_input_names{rhs.m_input_names} + , m_output_names{rhs.m_output_names} {} + const InputNames m_input_names; const OutputNames m_output_names; }; @@ -197,4 +210,3 @@ template constexpr bool is_input_v = detail::is_input::value; template constexpr bool is_output_v = detail::is_output::value; } // namespace algorithms - diff --git a/external/algorithms/core/include/algorithms/property.h b/external/algorithms/core/include/algorithms/property.h index 89c7e93a..57818c01 100644 --- a/external/algorithms/core/include/algorithms/property.h +++ b/external/algorithms/core/include/algorithms/property.h @@ -41,13 +41,20 @@ class PropertyMixin { class PropertyHandle; using PropertyMap = std::map; + // Copy constructor for the PropertyMixin forces new auto-registration of properties in + // copied object. The class using this mixin is responsible for copying over the Property + // contents. This is done instead of using the default copy constructor as depending on + // default copy behavior in the class using this mixin works for Properties but breaks in other + // cases (e.g. Resources where the pointer needs to be relocated) + PropertyMixin(const PropertyMixin& rhs) : m_props() { m_props.reserve(rhs.m_props); } + template void setProperty(std::string_view name, T&& value) { m_props.at(name).set(static_cast>(value)); } template T getProperty(std::string_view name) const { return std::get(m_props.at(name).get()); } - const PropertyMap& getProperties() const { return m_props; } + PropertyMap& getProperties const PropertyMap& getProperties() const { return m_props; } bool hasProperty(std::string_view name) const { return m_props.count(name) && m_props.at(name).hasValue(); } @@ -114,8 +121,8 @@ class PropertyMixin { set(static_cast(v)); } - Property() = delete; - Property(const Property&) = default; + Property() = delete; + Property(const Property&) = default; Property& operator=(const Property&) = default; // Only settable by explicitly calling the ::set() member function @@ -142,10 +149,10 @@ class PropertyMixin { template bool operator>=(const U& rhs) const { return m_value >= rhs; } template bool operator<(const U& rhs) const { return m_value < rhs; } template bool operator<=(const U& rhs) const { return m_value <= rhs; } - template decltype(auto) operator+(const U& rhs) const { return m_value + rhs; } - template decltype(auto) operator-(const U& rhs) const { return m_value - rhs; } - template decltype(auto) operator*(const U& rhs) const { return m_value * rhs; } - template decltype(auto) operator/(const U& rhs) const { return m_value / rhs; } + template decltype(auto) operator+(const U & rhs) const { return m_value + rhs; } + template decltype(auto) operator-(const U & rhs) const { return m_value - rhs; } + template decltype(auto) operator*(const U & rhs) const { return m_value * rhs; } + template decltype(auto) operator/(const U & rhs) const { return m_value / rhs; } // stl collection helpers if needed // forced to be templated so we only declare them when used @@ -157,8 +164,8 @@ class PropertyMixin { template decltype(auto) end() const { return value().end(); } template decltype(auto) begin() { return value().begin(); } template decltype(auto) end() { return value().end(); } - template decltype(auto) operator[](const Arg& arg) const { return value()[arg]; } - template decltype(auto) operator[](const Arg& arg) { return value()[arg]; } + template decltype(auto) operator[](const Arg & arg) const { return value()[arg]; } + template decltype(auto) operator[](const Arg & arg) { return value()[arg]; } template decltype(auto) find(const typename U::key_type& key) const { return value().find(key); diff --git a/external/algorithms/core/include/algorithms/resource.h b/external/algorithms/core/include/algorithms/resource.h index 303e7a3b..4b4d65bc 100644 --- a/external/algorithms/core/include/algorithms/resource.h +++ b/external/algorithms/core/include/algorithms/resource.h @@ -41,7 +41,7 @@ template class SvcResource : public NameMixin { }; // Mixin for Resource handling at the algorithm (or service) level (mirroring the -// PropertyMixin) +// PropertyMixin), also provides Context handling. class ResourceMixin { public: class ResourceHandle; @@ -50,7 +50,9 @@ class ResourceMixin { // Copy constructor for the ResourceMixin assumes new auto-registration (as // pointers need to be relocated to the new instance) - ResourceMixin(const ResourceMixin&) : m_resources() {} + ResourceMixin(const ResourceMixin& rhs) : m_resources() { + m_resources.reserve(rhs.m_resources.size()); + } ResourceMixin& operator=(const ResourceMixin& rhs) = delete;