From fd672b128d3d09dbc7e317adbd83b399a6b68a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 20 Mar 2023 16:03:02 +0100 Subject: [PATCH] Prepare Attributable for virtual inheritance Only one instance of std::shared, dynamically casted Use only zero-param constructors to avoid diamond initialization pitfalls --- include/openPMD/Iteration.hpp | 7 ++-- include/openPMD/RecordComponent.hpp | 24 ++++--------- include/openPMD/Series.hpp | 27 ++++++++------ include/openPMD/backend/Attributable.hpp | 9 +++-- include/openPMD/backend/BaseRecord.hpp | 35 ++++++------------- .../openPMD/backend/BaseRecordComponent.hpp | 24 ++++--------- include/openPMD/backend/Container.hpp | 31 +++++----------- .../openPMD/backend/PatchRecordComponent.hpp | 23 ++++-------- src/Iteration.cpp | 4 +-- src/Record.cpp | 4 +-- src/RecordComponent.cpp | 15 +++----- src/Series.cpp | 31 +++++++--------- src/backend/Attributable.cpp | 13 +++---- src/backend/BaseRecordComponent.cpp | 11 +++--- src/backend/PatchRecordComponent.cpp | 17 +++------ src/backend/Writable.cpp | 5 +-- 16 files changed, 100 insertions(+), 180 deletions(-) diff --git a/include/openPMD/Iteration.hpp b/include/openPMD/Iteration.hpp index 3c7fffc545..b84d6fc09c 100644 --- a/include/openPMD/Iteration.hpp +++ b/include/openPMD/Iteration.hpp @@ -244,17 +244,14 @@ class Iteration : public Attributable private: Iteration(); - std::shared_ptr m_iterationData{ - new internal::IterationData}; - inline internal::IterationData const &get() const { - return *m_iterationData; + return dynamic_cast(*m_attri); } inline internal::IterationData &get() { - return *m_iterationData; + return dynamic_cast(*m_attri); } void flushFileBased( diff --git a/include/openPMD/RecordComponent.hpp b/include/openPMD/RecordComponent.hpp index a4bc694e16..ad23dcebf1 100644 --- a/include/openPMD/RecordComponent.hpp +++ b/include/openPMD/RecordComponent.hpp @@ -105,8 +105,6 @@ class RecordComponent : public BaseRecordComponent friend class ParticleSpecies; template friend class BaseRecord; - template - friend class BaseRecordInterface; friend class Record; friend class Mesh; template @@ -432,31 +430,23 @@ class RecordComponent : public BaseRecordComponent */ bool dirtyRecursive() const; - std::shared_ptr m_recordComponentData{ - new internal::RecordComponentData()}; - - RecordComponent(); - // clang-format off OPENPMD_protected // clang-format on - RecordComponent(std::shared_ptr); + using Data_t = internal::RecordComponentData; - inline internal::RecordComponentData const &get() const - { - return *m_recordComponentData; - } + RecordComponent(); - inline internal::RecordComponentData &get() + inline Data_t const &get() const { - return *m_recordComponentData; + return dynamic_cast(*m_attri); } - inline void setData(std::shared_ptr data) + inline Data_t &get() { - m_recordComponentData = std::move(data); - BaseRecordComponent::setData(m_recordComponentData); + auto &res = dynamic_cast(*m_attri); + return res; } void readBase(); diff --git a/include/openPMD/Series.hpp b/include/openPMD/Series.hpp index 7b85986992..236b64d5e5 100644 --- a/include/openPMD/Series.hpp +++ b/include/openPMD/Series.hpp @@ -63,8 +63,13 @@ namespace internal * * (Not movable or copyable) * + * Class is final since our std::shared_ptr pattern has the little + * disadvantage that child constructors overwrite the parent constructors. + * Since the SeriesData constructor does some initialization, making this + * class final avoids stumbling over this pitfall. + * */ - class SeriesData : public AttributableData + class SeriesData final : public AttributableData { public: explicit SeriesData() = default; @@ -192,10 +197,6 @@ class Series : public Attributable friend class internal::SeriesData; friend class WriteIterations; -protected: - // Should not be called publicly, only by implementing classes - Series(std::shared_ptr); - public: explicit Series(); @@ -533,13 +534,11 @@ OPENPMD_private using iterations_t = decltype(internal::SeriesData::iterations); using iterations_iterator = iterations_t::iterator; - std::shared_ptr m_series = nullptr; - inline internal::SeriesData &get() { - if (m_series) + if (m_attri) { - return *m_series; + return dynamic_cast(*m_attri); } else { @@ -550,9 +549,9 @@ OPENPMD_private inline internal::SeriesData const &get() const { - if (m_series) + if (m_attri) { - return *m_series; + return dynamic_cast(*m_attri); } else { @@ -561,6 +560,12 @@ OPENPMD_private } } + inline void setData(std::shared_ptr series) + { + iterations = series->iterations; + Attributable::setData(std::move(series)); + } + std::unique_ptr parseInput(std::string); /** * @brief Parse non-backend-specific configuration in JSON config. diff --git a/include/openPMD/backend/Attributable.hpp b/include/openPMD/backend/Attributable.hpp index 90da6655bf..d64bd9f90a 100644 --- a/include/openPMD/backend/Attributable.hpp +++ b/include/openPMD/backend/Attributable.hpp @@ -114,9 +114,6 @@ class Attributable std::shared_ptr m_attri{ new internal::AttributableData()}; - // Should not be called publicly, only by implementing classes - Attributable(std::shared_ptr); - public: Attributable(); @@ -356,9 +353,11 @@ OPENPMD_protected return m_attri->m_writable; } - inline void setData(std::shared_ptr attri) + template + inline void setData(std::shared_ptr attri) { - m_attri = std::move(attri); + m_attri = std::static_pointer_cast( + std::move(attri)); } inline internal::AttributableData &get() diff --git a/include/openPMD/backend/BaseRecord.hpp b/include/openPMD/backend/BaseRecord.hpp index b35709b240..43064e6d05 100644 --- a/include/openPMD/backend/BaseRecord.hpp +++ b/include/openPMD/backend/BaseRecord.hpp @@ -63,30 +63,21 @@ class BaseRecord : public Container friend class Record; friend class Mesh; - std::shared_ptr > m_baseRecordData{ - new internal::BaseRecordData()}; + using Data_t = internal::BaseRecordData; + std::shared_ptr m_baseRecordData{new Data_t()}; - inline internal::BaseRecordData &get() + inline Data_t const &get() const { - return *m_baseRecordData; + return dynamic_cast(*this->m_attri); } - inline internal::BaseRecordData const &get() const + inline Data_t &get() { - return *m_baseRecordData; + return dynamic_cast(*this->m_attri); } BaseRecord(); -protected: - BaseRecord(std::shared_ptr >); - - inline void setData(internal::BaseRecordData *data) - { - m_baseRecordData = std::move(data); - Container::setData(m_baseRecordData); - } - public: using key_type = typename Container::key_type; using mapped_type = typename Container::mapped_type; @@ -137,7 +128,6 @@ class BaseRecord : public Container bool scalar() const; protected: - BaseRecord(internal::BaseRecordData *); void readBase(); private: @@ -164,7 +154,8 @@ namespace internal template BaseRecordData::BaseRecordData() { - Attributable impl{{this, [](auto const *) {}}}; + Attributable impl; + impl.setData({this, [](auto const *) {}}); impl.setAttribute( "unitDimension", std::array{{0., 0., 0., 0., 0., 0., 0.}}); @@ -172,17 +163,11 @@ namespace internal } // namespace internal template -BaseRecord::BaseRecord() : Container{nullptr} +BaseRecord::BaseRecord() { - Container::setData(m_baseRecordData); + Attributable::setData(std::make_shared()); } -template -BaseRecord::BaseRecord( - std::shared_ptr > data) - : Container{data}, m_baseRecordData{std::move(data)} -{} - template inline typename BaseRecord::mapped_type & BaseRecord::operator[](key_type const &key) diff --git a/include/openPMD/backend/BaseRecordComponent.hpp b/include/openPMD/backend/BaseRecordComponent.hpp index a2ea09e23e..56db7f3959 100644 --- a/include/openPMD/backend/BaseRecordComponent.hpp +++ b/include/openPMD/backend/BaseRecordComponent.hpp @@ -33,7 +33,7 @@ namespace openPMD { namespace internal { - class BaseRecordComponentData : public AttributableData + class BaseRecordComponentData : virtual public AttributableData { public: /** @@ -59,7 +59,7 @@ namespace internal }; } // namespace internal -class BaseRecordComponent : public Attributable +class BaseRecordComponent : virtual public Attributable { template friend class Container; @@ -100,28 +100,18 @@ class BaseRecordComponent : public Attributable ChunkTable availableChunks(); protected: - std::shared_ptr - m_baseRecordComponentData{new internal::BaseRecordComponentData()}; + using Data_t = internal::BaseRecordComponentData; - inline internal::BaseRecordComponentData const &get() const + inline Data_t const &get() const { - return *m_baseRecordComponentData; + return dynamic_cast(*m_attri); } - inline internal::BaseRecordComponentData &get() + inline Data_t &get() { - return *m_baseRecordComponentData; + return dynamic_cast(*m_attri); } - inline void setData(std::shared_ptr data) - { - m_baseRecordComponentData = std::move(data); - Attributable::setData(m_baseRecordComponentData); - } - - BaseRecordComponent(std::shared_ptr); - -private: BaseRecordComponent(); }; // BaseRecordComponent diff --git a/include/openPMD/backend/Container.hpp b/include/openPMD/backend/Container.hpp index c697593a12..0dea552f94 100644 --- a/include/openPMD/backend/Container.hpp +++ b/include/openPMD/backend/Container.hpp @@ -65,7 +65,7 @@ namespace internal typename T, typename T_key = std::string, typename T_container = std::map > - class ContainerData : public AttributableData + class ContainerData : virtual public AttributableData { public: using InternalContainer = T_container; @@ -128,7 +128,7 @@ template < typename T, typename T_key = std::string, typename T_container = std::map > -class Container : public Attributable +class Container : virtual public Attributable { static_assert( std::is_base_of::value, @@ -147,22 +147,14 @@ class Container : public Attributable using ContainerData = internal::ContainerData; using InternalContainer = T_container; - std::shared_ptr m_containerData{new ContainerData()}; - - inline void setData(std::shared_ptr containerData) - { - m_containerData = std::move(containerData); - Attributable::setData(m_containerData); - } - inline InternalContainer const &container() const { - return m_containerData->m_container; + return dynamic_cast(*m_attri).m_container; } inline InternalContainer &container() { - return m_containerData->m_container; + return dynamic_cast(*m_attri).m_container; } public: @@ -428,10 +420,6 @@ class Container : public Attributable OPENPMD_protected // clang-format on - Container(std::shared_ptr containerData) - : Attributable{containerData}, m_containerData{std::move(containerData)} - {} - void clear_unchecked() { if (written()) @@ -454,13 +442,9 @@ OPENPMD_protected flushAttributes(flushParams); } - // clang-format off -OPENPMD_private - // clang-format on - - Container() : Attributable{nullptr} + Container() { - Attributable::setData(m_containerData); + Attributable::setData(std::make_shared()); } }; @@ -532,7 +516,8 @@ namespace internal ~EraseStaleEntries() { auto &map = m_originalContainer.container(); - using iterator_t = typename BareContainer_t::const_iterator; + using iterator_t = + typename BareContainer_t::InternalContainer::const_iterator; std::vector deleteMe; deleteMe.reserve(map.size() - m_accessedKeys.size()); for (iterator_t it = map.begin(); it != map.end(); ++it) diff --git a/include/openPMD/backend/PatchRecordComponent.hpp b/include/openPMD/backend/PatchRecordComponent.hpp index 51537b0071..ba9eb0255b 100644 --- a/include/openPMD/backend/PatchRecordComponent.hpp +++ b/include/openPMD/backend/PatchRecordComponent.hpp @@ -111,32 +111,23 @@ OPENPMD_private */ bool dirtyRecursive() const; - std::shared_ptr - m_patchRecordComponentData{new internal::PatchRecordComponentData()}; - - PatchRecordComponent(); - // clang-format off OPENPMD_protected // clang-format on - PatchRecordComponent(std::shared_ptr); + using Data_t = internal::PatchRecordComponentData; - inline internal::PatchRecordComponentData const &get() const - { - return *m_patchRecordComponentData; - } + PatchRecordComponent(); - inline internal::PatchRecordComponentData &get() + inline Data_t const &get() const { - return *m_patchRecordComponentData; + return dynamic_cast(*m_attri); } - inline void - setData(std::shared_ptr data) + inline Data_t &get() { - m_patchRecordComponentData = std::move(data); - BaseRecordComponent::setData(m_patchRecordComponentData); + auto &res = dynamic_cast(*m_attri); + return res; } }; // PatchRecordComponent diff --git a/src/Iteration.cpp b/src/Iteration.cpp index 26ab93940e..9b816d642c 100644 --- a/src/Iteration.cpp +++ b/src/Iteration.cpp @@ -36,9 +36,9 @@ namespace openPMD using internal::CloseStatus; using internal::DeferredParseAccess; -Iteration::Iteration() : Attributable{nullptr} +Iteration::Iteration() { - Attributable::setData(m_iterationData); + Attributable::setData(std::make_shared()); setTime(static_cast(0)); setDt(static_cast(1)); setTimeUnitSI(1); diff --git a/src/Record.cpp b/src/Record.cpp index dc6949efdb..b886af2cd4 100644 --- a/src/Record.cpp +++ b/src/Record.cpp @@ -176,7 +176,5 @@ void Record::read() readAttributes(ReadMode::FullyReread); } -template <> -BaseRecord::mapped_type & -BaseRecord::operator[](std::string &&key); +template class BaseRecord; } // namespace openPMD diff --git a/src/RecordComponent.cpp b/src/RecordComponent.cpp index 164b38d127..4ad941e4c3 100644 --- a/src/RecordComponent.cpp +++ b/src/RecordComponent.cpp @@ -39,23 +39,16 @@ namespace internal { RecordComponentData::RecordComponentData() { - RecordComponent impl{ - std::shared_ptr{this, [](auto const *) {}}}; - impl.setUnitSI(1); - impl.resetDataset(Dataset(Datatype::CHAR, {1})); + m_dataset = Dataset(Datatype::CHAR, {1}); } } // namespace internal -RecordComponent::RecordComponent() : BaseRecordComponent{nullptr} +RecordComponent::RecordComponent() { - BaseRecordComponent::setData(m_recordComponentData); + Attributable::setData(std::make_shared()); + setUnitSI(1); } -RecordComponent::RecordComponent( - std::shared_ptr data) - : BaseRecordComponent{data}, m_recordComponentData{std::move(data)} -{} - // We need to instantiate this somewhere otherwise there might be linker issues // despite this thing actually being constepxr constexpr char const *const RecordComponent::SCALAR; diff --git a/src/Series.cpp b/src/Series.cpp index 779ba97906..2b17a13f30 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -2265,7 +2265,8 @@ namespace internal if (this->m_lastFlushSuccessful && m_writable.IOHandler && m_writable.IOHandler->has_value()) { - Series impl{{this, [](auto const *) {}}}; + Series impl; + impl.setData({this, [](auto const *) {}}); impl.flush(); impl.flushStep(/* doFlush = */ true); } @@ -2280,25 +2281,19 @@ namespace internal } } // namespace internal -Series::Series() : Attributable{nullptr}, iterations{} +Series::Series() : iterations{} {} -Series::Series(std::shared_ptr data) - : Attributable{data}, m_series{std::move(data)} -{ - iterations = m_series->iterations; -} - #if openPMD_HAVE_MPI Series::Series( std::string const &filepath, Access at, MPI_Comm comm, std::string const &options) - : Attributable{nullptr}, m_series{new internal::SeriesData} { - Attributable::setData(m_series); - iterations = m_series->iterations; + auto series = std::make_shared(); + iterations = series->iterations; + Attributable::setData(std::move(series)); json::TracingJSON optionsJson = json::parseOptions(options, comm, /* considerFiles = */ true); auto input = parseInput(filepath); @@ -2317,10 +2312,10 @@ Series::Series( Series::Series( std::string const &filepath, Access at, std::string const &options) - : Attributable{nullptr}, m_series{new internal::SeriesData} { - Attributable::setData(m_series); - iterations = m_series->iterations; + auto series = std::make_shared(); + iterations = series->iterations; + Attributable::setData(std::move(series)); json::TracingJSON optionsJson = json::parseOptions(options, /* considerFiles = */ true); auto input = parseInput(filepath); @@ -2333,15 +2328,16 @@ Series::Series( Series::operator bool() const { - return m_series.operator bool(); + return m_attri.operator bool(); } ReadIterations Series::readIterations() { // Use private constructor instead of copy constructor to avoid // object slicing - return { - this->m_series, IOHandler()->m_frontendAccess, get().m_parsePreference}; + Series res; + res.setData(std::dynamic_pointer_cast(this->m_attri)); + return {res, IOHandler()->m_frontendAccess, get().m_parsePreference}; } WriteIterations Series::writeIterations() @@ -2357,7 +2353,6 @@ WriteIterations Series::writeIterations() void Series::close() { get().close(); - m_series.reset(); m_attri.reset(); } diff --git a/src/backend/Attributable.cpp b/src/backend/Attributable.cpp index eb93888718..9ad17a77c2 100644 --- a/src/backend/Attributable.cpp +++ b/src/backend/Attributable.cpp @@ -39,10 +39,6 @@ namespace internal Attributable::Attributable() = default; -Attributable::Attributable(std::shared_ptr attri) - : m_attri{std::move(attri)} -{} - Attribute Attributable::getAttribute(std::string const &key) const { auto &attri = get(); @@ -120,7 +116,10 @@ Series Attributable::retrieveSeries() const } auto seriesData = &auxiliary::deref_dynamic_cast( findSeries->attributable); - return Series{{seriesData, [](auto const *) {}}}; + Series res; + res.setData( + std::shared_ptr{seriesData, [](auto const *) {}}); + return res; } Iteration const &Attributable::containingIteration() const @@ -199,7 +198,9 @@ auto Attributable::myPath() const -> MyPath std::reverse(res.group.begin(), res.group.end()); auto &seriesData = auxiliary::deref_dynamic_cast( findSeries->attributable); - Series series{{&seriesData, [](auto const *) {}}}; + Series series; + series.setData(std::shared_ptr{ + &seriesData, [](auto const *) {}}); res.seriesName = series.name(); res.seriesExtension = suffix(seriesData.m_format); res.directory = IOHandler()->directory; diff --git a/src/backend/BaseRecordComponent.cpp b/src/backend/BaseRecordComponent.cpp index 4460b7ede0..ab470e4950 100644 --- a/src/backend/BaseRecordComponent.cpp +++ b/src/backend/BaseRecordComponent.cpp @@ -65,13 +65,10 @@ ChunkTable BaseRecordComponent::availableChunks() return std::move(*param.chunks); } -BaseRecordComponent::BaseRecordComponent( - std::shared_ptr data) - : Attributable{data}, m_baseRecordComponentData{std::move(data)} -{} - -BaseRecordComponent::BaseRecordComponent() : Attributable{nullptr} +BaseRecordComponent::BaseRecordComponent() { - Attributable::setData(m_baseRecordComponentData); + Attributable::setData( + std::make_shared()); } + } // namespace openPMD diff --git a/src/backend/PatchRecordComponent.cpp b/src/backend/PatchRecordComponent.cpp index e1477ef7bd..edaa9646a1 100644 --- a/src/backend/PatchRecordComponent.cpp +++ b/src/backend/PatchRecordComponent.cpp @@ -27,11 +27,7 @@ namespace openPMD { namespace internal { - PatchRecordComponentData::PatchRecordComponentData() - { - PatchRecordComponent impl{{this, [](auto const *) {}}}; - impl.setUnitSI(1); - } + PatchRecordComponentData::PatchRecordComponentData() = default; } // namespace internal PatchRecordComponent &PatchRecordComponent::setUnitSI(double usi) @@ -70,16 +66,13 @@ Extent PatchRecordComponent::getExtent() const return get().m_dataset.extent; } -PatchRecordComponent::PatchRecordComponent() : BaseRecordComponent{nullptr} +PatchRecordComponent::PatchRecordComponent() { - BaseRecordComponent::setData(m_patchRecordComponentData); + Attributable::setData( + std::make_shared()); + setUnitSI(1); } -PatchRecordComponent::PatchRecordComponent( - std::shared_ptr data) - : BaseRecordComponent{data}, m_patchRecordComponentData{std::move(data)} -{} - void PatchRecordComponent::flush( std::string const &name, internal::FlushParams const &flushParams) { diff --git a/src/backend/Writable.cpp b/src/backend/Writable.cpp index f886e94046..725a8a86e1 100644 --- a/src/backend/Writable.cpp +++ b/src/backend/Writable.cpp @@ -49,8 +49,9 @@ void Writable::seriesFlush(std::string backendConfig) void Writable::seriesFlush(internal::FlushParams flushParams) { - auto series = - Attributable({attributable, [](auto const *) {}}).retrieveSeries(); + Attributable impl; + impl.setData({attributable, [](auto const *) {}}); + auto series = impl.retrieveSeries(); series.flush_impl( series.iterations.begin(), series.iterations.end(), flushParams); }