From f1a049ce1442ada7a4f56a973dd6acc49c4b6d51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Tich=C3=A1k?= Date: Thu, 3 Oct 2024 14:28:01 +0200 Subject: [PATCH] fix pointer usage in MO --- Framework/CMakeLists.txt | 6 +- .../include/QualityControl/MonitorObject.h | 31 +-- Framework/src/MonitorObject.cxx | 150 +++++++++++-- Framework/test/testMonitorObject.cxx | 202 ++++++++++++++++++ 4 files changed, 352 insertions(+), 37 deletions(-) diff --git a/Framework/CMakeLists.txt b/Framework/CMakeLists.txt index 104115d9b9..f29c28da94 100644 --- a/Framework/CMakeLists.txt +++ b/Framework/CMakeLists.txt @@ -22,10 +22,12 @@ target_include_directories( target_link_libraries(O2QualityControlTypes PUBLIC - ROOT::Hist + AliceO2::BookkeepingApi AliceO2::Common + AliceO2::InfoLogger O2::DataFormatsQualityControl - AliceO2::BookkeepingApi + ROOT::Hist + ) add_root_dictionary(O2QualityControlTypes diff --git a/Framework/include/QualityControl/MonitorObject.h b/Framework/include/QualityControl/MonitorObject.h index 8f3ba7ac91..2a1d58a327 100644 --- a/Framework/include/QualityControl/MonitorObject.h +++ b/Framework/include/QualityControl/MonitorObject.h @@ -50,12 +50,12 @@ class MonitorObject : public TObject /// Destructor ~MonitorObject() override; - /// Copy constructor - MonitorObject(const MonitorObject& other) = default; + // /// Copy constructor + MonitorObject(const MonitorObject& other); /// Move constructor MonitorObject(MonitorObject&& other) /*noexcept*/ = default; /// Copy assignment operator - MonitorObject& operator=(const MonitorObject& other) = default; + MonitorObject& operator=(const MonitorObject& other); /// Move assignment operator MonitorObject& operator=(MonitorObject&& other) /*noexcept*/ = default; @@ -69,19 +69,19 @@ class MonitorObject : public TObject /// \brief Return joined task name and name of the encapsulated object (if any). /// @return The name as "{getTaskName()}/{getName())}. - const std::string getFullName() const { return getTaskName() + "/" + getName(); } + std::string getFullName() const; - TObject* getObject() const { return mObject; } - void setObject(TObject* object) { mObject = object; } + TObject* getObject() const; + void setObject(TObject* object); - bool isIsOwner() const { return mIsOwner; } - void setIsOwner(bool isOwner) { mIsOwner = isOwner; } + bool isIsOwner() const; + void setIsOwner(bool isOwner); - const std::string& getTaskName() const { return mTaskName; } - void setTaskName(const std::string& taskName) { mTaskName = taskName; } + const std::string& getTaskName() const; + void setTaskName(const std::string& taskName); - const std::string& getDetectorName() const { return mDetectorName; } - void setDetectorName(const std::string& detectorName) { mDetectorName = detectorName; } + const std::string& getDetectorName() const; + void setDetectorName(const std::string& detectorName); const std::string& getTaskClass() const; void setTaskClass(const std::string& taskClass); @@ -117,6 +117,8 @@ class MonitorObject : public TObject void Draw(Option_t* option) override; TObject* DrawClone(Option_t* option) const override; + void Copy(TObject& object) const override; + /// \brief Build the path to this object. /// Build the path to this object as it will appear in the GUI. /// \return A string containing the path. @@ -126,7 +128,7 @@ class MonitorObject : public TObject void setDescription(const std::string& description); private: - TObject* mObject; + std::unique_ptr mObject; std::string mTaskName; std::string mTaskClass; std::string mDetectorName; @@ -141,6 +143,9 @@ class MonitorObject : public TObject // tells Merger to create an object with data from the last cycle only on the side of the complete object bool mCreateMovingWindow = false; + void releaseObject(); + void cloneAndSetObject(const MonitorObject&); + ClassDefOverride(MonitorObject, 12); }; diff --git a/Framework/src/MonitorObject.cxx b/Framework/src/MonitorObject.cxx index 2e1a0db2f5..bde7ceb7db 100644 --- a/Framework/src/MonitorObject.cxx +++ b/Framework/src/MonitorObject.cxx @@ -15,9 +15,11 @@ /// #include "QualityControl/MonitorObject.h" +#include +#include "QualityControl/RepoPathUtils.h" +#include "QualityControl/QcInfoLogger.h" #include -#include "QualityControl/RepoPathUtils.h" using namespace std; @@ -25,11 +27,8 @@ namespace o2::quality_control::core { MonitorObject::MonitorObject() - : TObject(), - mObject(nullptr), - mTaskName(""), - mDetectorName(""), - mIsOwner(true) + : TObject{}, + mIsOwner{ true } { mActivity.mProvenance = "qc"; mActivity.mId = 0; @@ -37,28 +36,71 @@ MonitorObject::MonitorObject() } MonitorObject::MonitorObject(TObject* object, const std::string& taskName, const std::string& taskClass, const std::string& detectorName, int runNumber, const std::string& periodName, const std::string& passName, const std::string& provenance) - : TObject(), - mObject(object), - mTaskName(taskName), - mTaskClass(taskClass), - mDetectorName(detectorName), - mActivity(runNumber, "NONE", periodName, passName, provenance, gInvalidValidityInterval), - mIsOwner(true) + : TObject{}, + mObject{ object }, + mTaskName{ taskName }, + mTaskClass{ taskClass }, + mDetectorName{ detectorName }, + mActivity{ runNumber, "NONE", periodName, passName, provenance, gInvalidValidityInterval }, + mIsOwner{ true } +{ +} + +MonitorObject::MonitorObject(const MonitorObject& other) + : TObject{ other }, + mObject{}, + mTaskName{ other.mTaskName }, + mTaskClass{ other.mTaskClass }, + mDetectorName{ other.mDetectorName }, + mUserMetadata{ other.mUserMetadata }, + mDescription{ other.mDescription }, + mActivity{ other.mActivity }, + mCreateMovingWindow{ other.mCreateMovingWindow } +{ + cloneAndSetObject(other); +} + +MonitorObject& MonitorObject::operator=(const MonitorObject& other) +{ + TObject::operator=(other); + mTaskName = other.mTaskName; + mTaskClass = other.mTaskClass; + mDetectorName = other.mDetectorName; + mUserMetadata = other.mUserMetadata; + mDescription = other.mDescription; + mActivity = other.mActivity; + mCreateMovingWindow = other.mCreateMovingWindow; + cloneAndSetObject(other); + + return *this; +} + +void MonitorObject::Copy(TObject& object) const { + static_cast(object) = *this; } MonitorObject::~MonitorObject() { - if (mIsOwner) { - delete mObject; - mObject = nullptr; - } + releaseObject(); } -void MonitorObject::Draw(Option_t* option) { mObject->Draw(option); } +void MonitorObject::Draw(Option_t* option) +{ + if (mObject) { + mObject->Draw(option); + } else { + ILOG(Error, Devel) << "MonitorObject::Draw() : You are trying to draw MonitorObject with no internal TObject" << ENDM; + } +} TObject* MonitorObject::DrawClone(Option_t* option) const { + if (!mObject) { + ILOG(Error, Devel) << "MonitorObject::DrawClone() : You are trying to draw MonitorObject with no internal TObject" << ENDM; + return nullptr; + } + auto* clone = new MonitorObject(); clone->setTaskName(this->getTaskName()); clone->setObject(mObject->DrawClone(option)); @@ -72,10 +114,9 @@ const std::string MonitorObject::getName() const const char* MonitorObject::GetName() const { - if (mObject == nullptr) { - cerr << "MonitorObject::getName() : No object in this MonitorObject, returning empty string" << endl; - static char empty[] = ""; - return empty; + if (!mObject) { + ILOG(Error, Ops) << "MonitorObject::getName() : No object in this MonitorObject, returning empty string" << ENDM; + return ""; } return mObject->GetName(); } @@ -160,6 +201,52 @@ void MonitorObject::updateValidity(validity_time_t value) mActivity.mValidity.update(value); } +std::string MonitorObject::getFullName() const +{ + return getTaskName() + "/" + getName(); +} + +TObject* MonitorObject::getObject() const +{ + return mObject.get(); +} + +void MonitorObject::setObject(TObject* object) +{ + releaseObject(); + mObject.reset(object); +} + +bool MonitorObject::isIsOwner() const +{ + return mIsOwner; +} + +void MonitorObject::setIsOwner(bool isOwner) +{ + mIsOwner = isOwner; +} + +const std::string& MonitorObject::getTaskName() const +{ + return mTaskName; +} + +void MonitorObject::setTaskName(const std::string& taskName) +{ + mTaskName = taskName; +} + +const std::string& MonitorObject::getDetectorName() const +{ + return mDetectorName; +} + +void MonitorObject::setDetectorName(const std::string& detectorName) +{ + mDetectorName = detectorName; +} + ValidityInterval MonitorObject::getValidity() const { return mActivity.mValidity; @@ -185,4 +272,23 @@ bool MonitorObject::getCreateMovingWindow() const return mCreateMovingWindow; } +void MonitorObject::releaseObject() +{ + if (!mIsOwner) { + void(mObject.release()); + } +} + +void MonitorObject::cloneAndSetObject(const MonitorObject& other) +{ + releaseObject(); + + if (auto* otherObject = other.getObject(); otherObject != nullptr && other.isIsOwner()) { + mObject.reset(otherObject->Clone()); + } else { + mObject.reset(otherObject); + } + mIsOwner = other.isIsOwner(); +} + } // namespace o2::quality_control::core diff --git a/Framework/test/testMonitorObject.cxx b/Framework/test/testMonitorObject.cxx index 70d05981c3..b35d3e96cc 100644 --- a/Framework/test/testMonitorObject.cxx +++ b/Framework/test/testMonitorObject.cxx @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -67,6 +68,207 @@ TEST_CASE("mo_save") gSystem->Unlink(filename.data()); } +TEST_CASE("mo_clone") +{ + auto th1 = std::make_unique("name", "title"); + TObject* obj = th1.get(); + auto* cloned = obj->Clone(); + auto* secondCloned = cloned->Clone(); + delete cloned; + delete secondCloned; +} + +TEST_CASE("mo_copy") +{ + auto compareWithoutObject = [](const MonitorObject& lhs, const MonitorObject& rhs) { + REQUIRE(lhs.getName() == rhs.getName()); + REQUIRE(lhs.getTaskName() == rhs.getTaskName()); + REQUIRE(lhs.getDetectorName() == rhs.getDetectorName()); + REQUIRE(lhs.getTaskClass() == rhs.getTaskClass()); + REQUIRE(lhs.isIsOwner() == rhs.isIsOwner()); + REQUIRE(lhs.getActivity() == rhs.getActivity()); + REQUIRE(lhs.getCreateMovingWindow() == rhs.getCreateMovingWindow()); + REQUIRE(lhs.getDescription() == rhs.getDescription()); + REQUIRE(lhs.getMetadataMap() == rhs.getMetadataMap()); + }; + + SECTION("Empty orignal MO") + { + MonitorObject original{}; + + SECTION("copy of non owning object returns same pointer") + { + auto compareShallowNonOwning = [&compareWithoutObject](const MonitorObject& lhs, const MonitorObject& rhs) { + compareWithoutObject(lhs, rhs); + // we expect shallow rhs when lhs does not own the underlying object + REQUIRE(lhs.getObject() == nullptr); + REQUIRE(rhs.getObject() == nullptr); + }; + + SECTION("copy constructor") + { + MonitorObject copy{ original }; + compareShallowNonOwning(original, copy); + } + + SECTION("copy assignment operator") + { + MonitorObject copy{}; + copy = original; + compareShallowNonOwning(original, copy); + } + + SECTION("Copy method") + { + MonitorObject copy{}; + original.Copy(copy); + compareShallowNonOwning(original, copy); + } + } + + SECTION("copy of object owned by MO returns deep copy, non nullptr init") + { + auto compareTNamed = [](const MonitorObject& lhs, const MonitorObject& rhs) { + REQUIRE(lhs.getObject() == nullptr); + REQUIRE(rhs.getObject() == nullptr); + }; + + SECTION("copy owning object before") + { + MonitorObject copy{}; + copy.setObject(new TNamed("copy named", "title copy")); + copy.setIsOwner(true); + + SECTION("copy assignment operator") + { + copy = original; + compareTNamed(original, copy); + } + + SECTION("Copy method") + { + original.Copy(copy); + compareTNamed(original, copy); + } + } + } + } + + SECTION("original MO with data") + { + + MonitorObject original{}; + + original.setTaskName("taskName"); + original.setTaskClass("taskClass"); + original.setDescription("description"); + original.setDetectorName("TST"); + original.setActivity({ 123, "type", "periodName", "passName", "provenance", gFullValidityInterval, "beamType", "partitionName", 2 }); + original.setCreateMovingWindow(true); + + SECTION("copy of non owning object returns same pointer") + { + auto compareShallowNonOwning = [&compareWithoutObject](const MonitorObject& lhs, const MonitorObject& rhs) { + compareWithoutObject(lhs, rhs); + // we expect shallow rhs when lhs does not own the underlying object + REQUIRE((lhs.getObject() != nullptr && lhs.getObject() == rhs.getObject())); + }; + + auto th1 = TH1I("name", "title", 10, 0, 10); + th1.Fill(8); + original.setObject(&th1); + original.setIsOwner(false); + + SECTION("copy constructor") + { + MonitorObject copy{ original }; + compareShallowNonOwning(original, copy); + } + + SECTION("copy assignment operator") + { + MonitorObject copy{}; + copy = original; + compareShallowNonOwning(original, copy); + } + + SECTION("Copy method") + { + MonitorObject copy{}; + original.Copy(copy); + compareShallowNonOwning(original, copy); + } + } + + SECTION("copy of object owned by MO returns deep copy, init from nullptr") + { + auto compareTNamed = [](const MonitorObject& lhs, const MonitorObject& rhs) { + auto* namedoriginal = static_cast(lhs.getObject()); + auto* namedcopy = static_cast(rhs.getObject()); + REQUIRE(std::string(namedoriginal->GetName()) == std::string(namedcopy->GetName())); + REQUIRE(std::string(namedoriginal->GetTitle()) == std::string(namedcopy->GetTitle())); + }; + + auto* named = new TNamed("named", "title"); + original.setObject(named); + original.setIsOwner(true); + + SECTION("copy constructor") + { + MonitorObject copy{ original }; + compareTNamed(original, copy); + } + + SECTION("copy assignment operator") + { + MonitorObject copy{}; + copy = original; + compareTNamed(original, copy); + } + + SECTION("Copy method") + { + MonitorObject copy{}; + original.Copy(copy); + compareTNamed(original, copy); + } + } + + SECTION("copy of object owned by MO returns deep copy, non nullptr init") + { + auto compareTNamed = [](const MonitorObject& lhs, const MonitorObject& rhs) { + auto* namedoriginal = static_cast(lhs.getObject()); + auto* namedcopy = static_cast(rhs.getObject()); + REQUIRE(std::string(namedoriginal->GetName()) == std::string(namedcopy->GetName())); + REQUIRE(std::string(namedoriginal->GetTitle()) == std::string(namedcopy->GetTitle())); + }; + + auto* named = new TNamed("named", "title"); + original.setObject(named); + original.setIsOwner(true); + + SECTION("copy owning object before") + { + MonitorObject copy{}; + copy.setObject(new TNamed("copy named", "title copy")); + copy.setIsOwner(true); + + SECTION("copy assignment operator") + { + copy = original; + compareTNamed(original, copy); + } + + SECTION("Copy method") + { + original.Copy(copy); + compareTNamed(original, copy); + } + } + } + } +} + TEST_CASE("metadata") { string objectName = "asdf";