From 8755ae931dd3cc4e165af32b02fb3004efd71986 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 15:42:13 +0100 Subject: [PATCH 01/13] test(Base): Add basic test coverage for `FairVolumeList` --- tests/base/CMakeLists.txt | 9 ++--- tests/base/dummy_example.cxx | 49 -------------------------- tests/base/sim/test_FairVolumeList.cxx | 47 ++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 55 deletions(-) delete mode 100644 tests/base/dummy_example.cxx create mode 100644 tests/base/sim/test_FairVolumeList.cxx diff --git a/tests/base/CMakeLists.txt b/tests/base/CMakeLists.txt index ce505ec031..7ea4526995 100644 --- a/tests/base/CMakeLists.txt +++ b/tests/base/CMakeLists.txt @@ -1,5 +1,5 @@ ################################################################################ -# Copyright (C) 2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH # +# Copyright (C) 2023-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH # # # # This software is distributed under the terms of the # # GNU Lesser General Public Licence (LGPL) version 3, # @@ -8,10 +8,7 @@ fairroot_add_catch2_test_suite(Base SOURCES - dummy_example.cxx # remove me once first real tests are added - # add more test files if appropriate + sim/test_FairVolumeList.cxx - # DEPENDENCIES - # FairRoot::Base - # list all the targets to build the test suite here + DEPENDENCIES FairRoot::Base ) diff --git a/tests/base/dummy_example.cxx b/tests/base/dummy_example.cxx deleted file mode 100644 index da797787a9..0000000000 --- a/tests/base/dummy_example.cxx +++ /dev/null @@ -1,49 +0,0 @@ -/******************************************************************************** - * Copyright (C) 2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * - * * - * This software is distributed under the terms of the * - * GNU Lesser General Public Licence (LGPL) version 3, * - * copied verbatim in the file "LICENSE" * - ********************************************************************************/ - -#include -#include - -// shamelessly stolen from https://github.com/catchorg/Catch2/blob/devel/docs/tutorial.md#test-cases-and-sections -TEST_CASE("vectors can be sized and resized") -{ - - std::vector v(5); - - REQUIRE(v.size() == 5); - REQUIRE(v.capacity() >= 5); - - SECTION("resizing bigger changes size and capacity") - { - v.resize(10); - - REQUIRE(v.size() == 10); - REQUIRE(v.capacity() >= 10); - } - SECTION("resizing smaller changes size but not capacity") - { - v.resize(0); - - REQUIRE(v.size() == 0); - REQUIRE(v.capacity() >= 5); - } - SECTION("reserving bigger changes capacity but not size") - { - v.reserve(10); - - REQUIRE(v.size() == 5); - REQUIRE(v.capacity() >= 10); - } - SECTION("reserving smaller does not change size or capacity") - { - v.reserve(0); - - REQUIRE(v.size() == 5); - REQUIRE(v.capacity() >= 5); - } -} diff --git a/tests/base/sim/test_FairVolumeList.cxx b/tests/base/sim/test_FairVolumeList.cxx new file mode 100644 index 0000000000..517ade8160 --- /dev/null +++ b/tests/base/sim/test_FairVolumeList.cxx @@ -0,0 +1,47 @@ +/******************************************************************************** + * Copyright (C) 2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * + * * + * This software is distributed under the terms of the * + * GNU Lesser General Public Licence (LGPL) version 3, * + * copied verbatim in the file "LICENSE" * + ********************************************************************************/ + +#include +#include +#include +#include +#include +#include + +static auto MakeVolume(std::string_view name) +{ + return std::make_unique(TString{name}); +} + +TEST_CASE("FairVolumeList") +{ + FairVolumeList volumes; + + SECTION("is a set") + { + auto a = MakeVolume("a"); + auto* a_ptr = a.get(); + auto a_name = TString(a->GetName()); + + volumes.addVolume(a.release()); + volumes.addVolume(MakeVolume("b").release()); + volumes.addVolume(MakeVolume("a").release()); + + REQUIRE(volumes.getEntries() == 2); + REQUIRE(volumes.getVolume(&a_name) == a_ptr); + } + + SECTION("handles reading a non-existent volume") + { + TString z = "z"; + constexpr auto notFound = -111; + + REQUIRE(volumes.getVolume(&z) == nullptr); + REQUIRE(volumes.getVolumeId(&z) == notFound); + } +} From 5c3d912fb0fa9777ac55d1dde6f5dbd2ae38966c Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 15:50:03 +0100 Subject: [PATCH 02/13] feat(Base): Eliminate extra allocation in `FairVolumeList` * Explicitely delete copy ctor and assignment which were previously private and unimplemented * Disable unused streamer --- fairroot/base/sim/FairVolumeList.cxx | 19 +++---------------- fairroot/base/sim/FairVolumeList.h | 18 +++++++++--------- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index a2158cfcab..6d4b090e5b 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -15,19 +15,6 @@ #include "FairLogger.h" // for logging #include "FairVolume.h" // for FairVolume -FairVolumeList::FairVolumeList() - : TObject() - , fData(new TObjArray()) -{} - -FairVolumeList::~FairVolumeList() -{ - if (fData) { - fData->Delete(); - delete fData; - } -} - FairVolume* FairVolumeList::getVolume(TString* name) { @@ -54,8 +41,8 @@ FairVolume* FairVolumeList::findObject(TString name) { FairVolume* obj = nullptr; - for (int i = 0; i < fData->GetEntriesFast(); i++) { - obj = static_cast(fData->At(i)); + for (int i = 0; i < fData.GetEntriesFast(); i++) { + obj = static_cast(fData.At(i)); if (obj) { if (obj->GetName() == name) { return obj; @@ -74,6 +61,6 @@ void FairVolumeList::addVolume(FairVolume* elem) LOG(error) << "FairVolumeList element: " << elem->GetName() << " VolId : " << elem->getVolumeId() << " already defined " << v->getVolumeId(); } else { - fData->Add(elem); + fData.Add(elem); } } diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index ebca2ea32c..adc15cdcf2 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * + * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * * * * This software is distributed under the terms of the * * GNU Lesser General Public Licence (LGPL) version 3, * @@ -25,13 +25,13 @@ class FairVolumeList : public TObject { private: - TObjArray* fData; - FairVolumeList(const FairVolumeList&); - FairVolumeList& operator=(const FairVolumeList&); + TObjArray fData; public: - FairVolumeList(); - ~FairVolumeList() override; + FairVolumeList() { fData.SetOwner(kTRUE); } + FairVolumeList(const FairVolumeList&) = delete; + FairVolumeList& operator=(const FairVolumeList&) = delete; + ~FairVolumeList() override = default; FairVolume* getVolume(TString* name); Int_t getVolumeId(TString* name); @@ -39,10 +39,10 @@ class FairVolumeList : public TObject FairVolume* findObject(TString name); void addVolume(FairVolume* elem); - Int_t getEntries() { return fData->GetEntries(); } - FairVolume* At(Int_t pos) { return (dynamic_cast(fData->At(pos))); } + Int_t getEntries() { return fData.GetEntries(); } + FairVolume* At(Int_t pos) { return (dynamic_cast(fData.At(pos))); } - ClassDefOverride(FairVolumeList, 1); + ClassDefOverride(FairVolumeList, 0); }; #endif // FAIR_VOLUMELIST_H From 82e7b5ad1b7d785e970aa35f7406483529ae2386 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 15:57:38 +0100 Subject: [PATCH 03/13] feat(Base): Replace magic number with named constant --- fairroot/base/sim/FairVolumeList.cxx | 8 ++++---- fairroot/base/sim/FairVolumeList.h | 2 ++ tests/base/sim/test_FairVolumeList.cxx | 3 +-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index 6d4b090e5b..6739bc5edb 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -30,11 +30,11 @@ Int_t FairVolumeList::getVolumeId(TString* name) { FairVolume* vol = getVolume(name); - if (vol) { - return vol->getVolumeId(); - } else { - return -111; + if (!vol) { + return fgkNotFound; } + + return vol->getVolumeId(); } FairVolume* FairVolumeList::findObject(TString name) diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index adc15cdcf2..4208ba7eec 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -33,6 +33,8 @@ class FairVolumeList : public TObject FairVolumeList& operator=(const FairVolumeList&) = delete; ~FairVolumeList() override = default; + static constexpr Int_t fgkNotFound = -111; + FairVolume* getVolume(TString* name); Int_t getVolumeId(TString* name); diff --git a/tests/base/sim/test_FairVolumeList.cxx b/tests/base/sim/test_FairVolumeList.cxx index 517ade8160..bf74e1e01f 100644 --- a/tests/base/sim/test_FairVolumeList.cxx +++ b/tests/base/sim/test_FairVolumeList.cxx @@ -39,9 +39,8 @@ TEST_CASE("FairVolumeList") SECTION("handles reading a non-existent volume") { TString z = "z"; - constexpr auto notFound = -111; REQUIRE(volumes.getVolume(&z) == nullptr); - REQUIRE(volumes.getVolumeId(&z) == notFound); + REQUIRE(volumes.getVolumeId(&z) == FairVolumeList::fgkNotFound); } } From dac92e18ee53e91cfafa2f0bcf52e21b296e2f17 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 16:29:28 +0100 Subject: [PATCH 04/13] feat(Base): Fix string arguments * Passing a string ptr and then dereferencing without checking for `nullptr` clearly shows that the intention was to use a C++ reference * Passing a string by value is making an unnecessary copy which can be avoided here. --- fairroot/base/sim/FairVolumeList.cxx | 10 +++++----- fairroot/base/sim/FairVolumeList.h | 20 ++++++++++++++++---- tests/base/sim/test_FairVolumeList.cxx | 9 +++------ 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index 6739bc5edb..842d167e02 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -15,18 +15,18 @@ #include "FairLogger.h" // for logging #include "FairVolume.h" // for FairVolume -FairVolume* FairVolumeList::getVolume(TString* name) +FairVolume* FairVolumeList::getVolume(const TString& name) { + TObject* obj = findObject(name); - TObject* obj = findObject(*name); if (obj) { - LOG(info) << "FairVolume getVolume " << name->Data() << "found"; + LOG(info) << "FairVolume getVolume " << name << " found"; } return static_cast(obj); } -Int_t FairVolumeList::getVolumeId(TString* name) +Int_t FairVolumeList::getVolumeId(const TString& name) { FairVolume* vol = getVolume(name); @@ -37,7 +37,7 @@ Int_t FairVolumeList::getVolumeId(TString* name) return vol->getVolumeId(); } -FairVolume* FairVolumeList::findObject(TString name) +FairVolume* FairVolumeList::findObject(const TString& name) { FairVolume* obj = nullptr; diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index 4208ba7eec..de13a3d438 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -14,6 +14,7 @@ #include // for TObjArray #include // for TObject #include // for TString +#include // for assert /** * This Object is only used for internal book keeping! @@ -35,10 +36,21 @@ class FairVolumeList : public TObject static constexpr Int_t fgkNotFound = -111; - FairVolume* getVolume(TString* name); - Int_t getVolumeId(TString* name); - - FairVolume* findObject(TString name); + FairVolume* getVolume(const TString& name); + [[deprecated]] FairVolume* getVolume(TString* name) + { + assert(name); + return getVolume(*name); + } + + Int_t getVolumeId(const TString& name); + [[deprecated]] Int_t getVolumeId(TString* name) + { + assert(name); + return getVolumeId(*name); + } + + FairVolume* findObject(const TString& name); void addVolume(FairVolume* elem); Int_t getEntries() { return fData.GetEntries(); } diff --git a/tests/base/sim/test_FairVolumeList.cxx b/tests/base/sim/test_FairVolumeList.cxx index bf74e1e01f..d51ad71e08 100644 --- a/tests/base/sim/test_FairVolumeList.cxx +++ b/tests/base/sim/test_FairVolumeList.cxx @@ -26,21 +26,18 @@ TEST_CASE("FairVolumeList") { auto a = MakeVolume("a"); auto* a_ptr = a.get(); - auto a_name = TString(a->GetName()); volumes.addVolume(a.release()); volumes.addVolume(MakeVolume("b").release()); volumes.addVolume(MakeVolume("a").release()); REQUIRE(volumes.getEntries() == 2); - REQUIRE(volumes.getVolume(&a_name) == a_ptr); + REQUIRE(volumes.getVolume("a") == a_ptr); } SECTION("handles reading a non-existent volume") { - TString z = "z"; - - REQUIRE(volumes.getVolume(&z) == nullptr); - REQUIRE(volumes.getVolumeId(&z) == FairVolumeList::fgkNotFound); + REQUIRE(volumes.getVolume("z") == nullptr); + REQUIRE(volumes.getVolumeId("z") == FairVolumeList::fgkNotFound); } } From 7f0f7f185cbef80298aeedb368631d638c1782d6 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 17:03:18 +0100 Subject: [PATCH 05/13] feat(Base): Prevent memory leak with duplicates in `FairVolumeList` The raw pointer arg is transferring ownership. * In case the given object was not added to the list, it just leaked. * A new overload with a unique_ptr arg avoids the leak and communicates the transfer of ownership. --- fairroot/base/sim/FairModule.cxx | 32 +++++++++++++------------- fairroot/base/sim/FairVolumeList.cxx | 12 +++++----- fairroot/base/sim/FairVolumeList.h | 5 +++- tests/base/sim/test_FairVolumeList.cxx | 6 ++--- 4 files changed, 29 insertions(+), 26 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index 0d15912d5e..397a72157f 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -213,7 +213,6 @@ void FairModule::ProcessNodes(TList* aList) TListIter iter(aList); FairGeoNode* node = nullptr; FairGeoNode* MotherNode = nullptr; - FairVolume* volume = nullptr; FairRuntimeDb* rtdb = FairRun::Instance()->GetRuntimeDb(); FairGeoParSet* par = static_cast(rtdb->getContainer("FairGeoParSet")); TObjArray* fNodes = par->GetGeoNodes(); @@ -221,22 +220,23 @@ void FairModule::ProcessNodes(TList* aList) node->calcLabTransform(); MotherNode = node->getMotherNode(); - volume = new FairVolume(node->getTruncName(), fNbOfVolumes++); + auto volume = std::make_unique(node->getTruncName(), fNbOfVolumes++); + auto volume_ptr = volume.get(); volume->setRealName(node->GetName()); - vList->addVolume(volume); - volume->setGeoNode(node); - volume->setCopyNo(node->getCopyNo()); + vList->addVolume(std::move(volume)); + volume_ptr->setGeoNode(node); + volume_ptr->setCopyNo(node->getCopyNo()); if (MotherNode != 0) { - volume->setMotherId(node->getMCid()); - volume->setMotherCopyNo(MotherNode->getCopyNo()); + volume_ptr->setMotherId(node->getMCid()); + volume_ptr->setMotherCopyNo(MotherNode->getCopyNo()); } FairGeoVolume* aVol = nullptr; if (node->isSensitive() && fActive) { - volume->setModId(fModId); - volume->SetModule(this); - fAllSensitiveVolumes.push_back(volume); + volume_ptr->setModId(fModId); + volume_ptr->SetModule(this); + fAllSensitiveVolumes.push_back(volume_ptr); aVol = dynamic_cast(node); fNodes->AddLast(aVol); fNbOfSensitiveVol++; @@ -251,12 +251,12 @@ void FairModule::AddSensitiveVolume(TGeoVolume* v) // Only register volumes which are not already registered // Otherwise the stepping will be slowed down if (!vList->findObject(v->GetName())) { - FairVolume* volume = nullptr; - volume = new FairVolume(v->GetName(), fNbOfVolumes++); - vList->addVolume(volume); - volume->setModId(fModId); - volume->SetModule(this); - fAllSensitiveVolumes.push_back(volume); + auto volume = std::make_unique(v->GetName(), fNbOfVolumes++); + auto volume_ptr = volume.get(); + vList->addVolume(std::move(volume)); + volume_ptr->setModId(fModId); + volume_ptr->SetModule(this); + fAllSensitiveVolumes.push_back(volume_ptr); fNbOfSensitiveVol++; } } diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index 842d167e02..909f617bac 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -53,14 +53,14 @@ FairVolume* FairVolumeList::findObject(const TString& name) return nullptr; } -void FairVolumeList::addVolume(FairVolume* elem) +void FairVolumeList::addVolume(std::unique_ptr vol) { - FairVolume* v = findObject(elem->GetName()); + FairVolume* vol_found = findObject(vol->GetName()); - if (v) { - LOG(error) << "FairVolumeList element: " << elem->GetName() << " VolId : " << elem->getVolumeId() - << " already defined " << v->getVolumeId(); + if (vol_found) { + LOG(error) << "FairVolumeList element: " << vol->GetName() << " VolId : " << vol->getVolumeId() + << " already defined " << vol_found->getVolumeId(); } else { - fData.Add(elem); + fData.Add(vol.release()); } } diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index de13a3d438..ed2bfb4bd1 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -15,6 +15,7 @@ #include // for TObject #include // for TString #include // for assert +#include // for std::unique_ptr /** * This Object is only used for internal book keeping! @@ -51,7 +52,9 @@ class FairVolumeList : public TObject } FairVolume* findObject(const TString& name); - void addVolume(FairVolume* elem); + + void addVolume(std::unique_ptr vol); + [[deprecated]] void addVolume(FairVolume* elem) { addVolume(std::unique_ptr(elem)); } Int_t getEntries() { return fData.GetEntries(); } FairVolume* At(Int_t pos) { return (dynamic_cast(fData.At(pos))); } diff --git a/tests/base/sim/test_FairVolumeList.cxx b/tests/base/sim/test_FairVolumeList.cxx index d51ad71e08..64812334bb 100644 --- a/tests/base/sim/test_FairVolumeList.cxx +++ b/tests/base/sim/test_FairVolumeList.cxx @@ -27,9 +27,9 @@ TEST_CASE("FairVolumeList") auto a = MakeVolume("a"); auto* a_ptr = a.get(); - volumes.addVolume(a.release()); - volumes.addVolume(MakeVolume("b").release()); - volumes.addVolume(MakeVolume("a").release()); + volumes.addVolume(std::move(a)); + volumes.addVolume(MakeVolume("b")); + volumes.addVolume(MakeVolume("a")); REQUIRE(volumes.getEntries() == 2); REQUIRE(volumes.getVolume("a") == a_ptr); From 4a0e753ddfa2c28440355c536192a3ece7def554 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 17:36:16 +0100 Subject: [PATCH 06/13] fix(Base): Do not reimplement existing functions --- fairroot/base/sim/FairVolumeList.cxx | 16 ---------------- fairroot/base/sim/FairVolumeList.h | 2 +- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index 909f617bac..cd424cfe29 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -37,22 +37,6 @@ Int_t FairVolumeList::getVolumeId(const TString& name) return vol->getVolumeId(); } -FairVolume* FairVolumeList::findObject(const TString& name) -{ - FairVolume* obj = nullptr; - - for (int i = 0; i < fData.GetEntriesFast(); i++) { - obj = static_cast(fData.At(i)); - if (obj) { - if (obj->GetName() == name) { - return obj; - } - } - } - - return nullptr; -} - void FairVolumeList::addVolume(std::unique_ptr vol) { FairVolume* vol_found = findObject(vol->GetName()); diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index ed2bfb4bd1..9061ba123b 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -51,7 +51,7 @@ class FairVolumeList : public TObject return getVolumeId(*name); } - FairVolume* findObject(const TString& name); + FairVolume* findObject(const TString& name) { return static_cast(fData.FindObject(name.Data())); } void addVolume(std::unique_ptr vol); [[deprecated]] void addVolume(FairVolume* elem) { addVolume(std::unique_ptr(elem)); } From 4e4a3b6060ec4c1749b6ef164ef9e11d66e59ef8 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 17:41:38 +0100 Subject: [PATCH 07/13] fix(Base): Simplify `FairVolumeList` further * Avoid unnecessary expliciteness and track return value types with `auto` * Use conditional operator for readability --- fairroot/base/sim/FairVolumeList.cxx | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index cd424cfe29..208443bb65 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -12,12 +12,9 @@ #include "FairVolumeList.h" -#include "FairLogger.h" // for logging -#include "FairVolume.h" // for FairVolume - FairVolume* FairVolumeList::getVolume(const TString& name) { - TObject* obj = findObject(name); + auto obj = findObject(name); if (obj) { LOG(info) << "FairVolume getVolume " << name << " found"; @@ -28,18 +25,14 @@ FairVolume* FairVolumeList::getVolume(const TString& name) Int_t FairVolumeList::getVolumeId(const TString& name) { - FairVolume* vol = getVolume(name); - - if (!vol) { - return fgkNotFound; - } + auto vol = getVolume(name); - return vol->getVolumeId(); + return vol ? vol->getVolumeId() : fgkNotFound; } void FairVolumeList::addVolume(std::unique_ptr vol) { - FairVolume* vol_found = findObject(vol->GetName()); + auto vol_found = findObject(vol->GetName()); if (vol_found) { LOG(error) << "FairVolumeList element: " << vol->GetName() << " VolId : " << vol->getVolumeId() From f8525850592d31cbe445ecaf87cd37e641714809 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 18:20:36 +0100 Subject: [PATCH 08/13] feat(Base): Return added volume or `nullptr` --- fairroot/base/sim/FairModule.cxx | 36 ++++++++++++++------------ fairroot/base/sim/FairVolumeList.cxx | 9 ++++--- fairroot/base/sim/FairVolumeList.h | 4 +-- tests/base/sim/test_FairVolumeList.cxx | 15 ++++++++--- 4 files changed, 40 insertions(+), 24 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index 397a72157f..e98eab7a11 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -37,6 +37,7 @@ #include // for TObjArray #include // for TObject #include // for TSystem, gSystem +#include #ifdef ROOT_HAS_GDML #include @@ -220,23 +221,28 @@ void FairModule::ProcessNodes(TList* aList) node->calcLabTransform(); MotherNode = node->getMotherNode(); - auto volume = std::make_unique(node->getTruncName(), fNbOfVolumes++); - auto volume_ptr = volume.get(); + auto nodeTruncName = node->getTruncName(); + auto volume = std::make_unique(nodeTruncName, fNbOfVolumes++); volume->setRealName(node->GetName()); - vList->addVolume(std::move(volume)); - volume_ptr->setGeoNode(node); - volume_ptr->setCopyNo(node->getCopyNo()); + auto addedVol = vList->addVolume(std::move(volume)); + if (!addedVol) { + LOG(warn) << "Skipping further processing of FairGeoNode " << nodeTruncName + << ". It is already present in the global volume list."; + continue; + } + addedVol->setGeoNode(node); + addedVol->setCopyNo(node->getCopyNo()); if (MotherNode != 0) { - volume_ptr->setMotherId(node->getMCid()); - volume_ptr->setMotherCopyNo(MotherNode->getCopyNo()); + addedVol->setMotherId(node->getMCid()); + addedVol->setMotherCopyNo(MotherNode->getCopyNo()); } FairGeoVolume* aVol = nullptr; if (node->isSensitive() && fActive) { - volume_ptr->setModId(fModId); - volume_ptr->SetModule(this); - fAllSensitiveVolumes.push_back(volume_ptr); + addedVol->setModId(fModId); + addedVol->SetModule(this); + fAllSensitiveVolumes.push_back(addedVol); aVol = dynamic_cast(node); fNodes->AddLast(aVol); fNbOfSensitiveVol++; @@ -251,12 +257,10 @@ void FairModule::AddSensitiveVolume(TGeoVolume* v) // Only register volumes which are not already registered // Otherwise the stepping will be slowed down if (!vList->findObject(v->GetName())) { - auto volume = std::make_unique(v->GetName(), fNbOfVolumes++); - auto volume_ptr = volume.get(); - vList->addVolume(std::move(volume)); - volume_ptr->setModId(fModId); - volume_ptr->SetModule(this); - fAllSensitiveVolumes.push_back(volume_ptr); + auto addedVol = vList->addVolume(std::make_unique(v->GetName(), fNbOfVolumes++)); + addedVol->setModId(fModId); + addedVol->SetModule(this); + fAllSensitiveVolumes.push_back(addedVol); fNbOfSensitiveVol++; } } diff --git a/fairroot/base/sim/FairVolumeList.cxx b/fairroot/base/sim/FairVolumeList.cxx index 208443bb65..519cd9b39f 100644 --- a/fairroot/base/sim/FairVolumeList.cxx +++ b/fairroot/base/sim/FairVolumeList.cxx @@ -30,14 +30,17 @@ Int_t FairVolumeList::getVolumeId(const TString& name) return vol ? vol->getVolumeId() : fgkNotFound; } -void FairVolumeList::addVolume(std::unique_ptr vol) +FairVolume* FairVolumeList::addVolume(std::unique_ptr vol) { auto vol_found = findObject(vol->GetName()); if (vol_found) { LOG(error) << "FairVolumeList element: " << vol->GetName() << " VolId : " << vol->getVolumeId() << " already defined " << vol_found->getVolumeId(); - } else { - fData.Add(vol.release()); + return nullptr; } + + auto vol_added = vol.get(); + fData.Add(vol.release()); + return vol_added; } diff --git a/fairroot/base/sim/FairVolumeList.h b/fairroot/base/sim/FairVolumeList.h index 9061ba123b..76a3e23af5 100644 --- a/fairroot/base/sim/FairVolumeList.h +++ b/fairroot/base/sim/FairVolumeList.h @@ -53,8 +53,8 @@ class FairVolumeList : public TObject FairVolume* findObject(const TString& name) { return static_cast(fData.FindObject(name.Data())); } - void addVolume(std::unique_ptr vol); - [[deprecated]] void addVolume(FairVolume* elem) { addVolume(std::unique_ptr(elem)); } + [[nodiscard]] FairVolume* addVolume(std::unique_ptr vol); + [[deprecated]] FairVolume* addVolume(FairVolume* elem) { return addVolume(std::unique_ptr(elem)); } Int_t getEntries() { return fData.GetEntries(); } FairVolume* At(Int_t pos) { return (dynamic_cast(fData.At(pos))); } diff --git a/tests/base/sim/test_FairVolumeList.cxx b/tests/base/sim/test_FairVolumeList.cxx index 64812334bb..7b7badcf72 100644 --- a/tests/base/sim/test_FairVolumeList.cxx +++ b/tests/base/sim/test_FairVolumeList.cxx @@ -27,9 +27,9 @@ TEST_CASE("FairVolumeList") auto a = MakeVolume("a"); auto* a_ptr = a.get(); - volumes.addVolume(std::move(a)); - volumes.addVolume(MakeVolume("b")); - volumes.addVolume(MakeVolume("a")); + REQUIRE(volumes.addVolume(std::move(a))); + REQUIRE(volumes.addVolume(MakeVolume("b"))); + REQUIRE(volumes.addVolume(MakeVolume("a")) == nullptr); REQUIRE(volumes.getEntries() == 2); REQUIRE(volumes.getVolume("a") == a_ptr); @@ -40,4 +40,13 @@ TEST_CASE("FairVolumeList") REQUIRE(volumes.getVolume("z") == nullptr); REQUIRE(volumes.getVolumeId("z") == FairVolumeList::fgkNotFound); } + + SECTION("adding volumes returns added vol or nullptr") + { + auto a = MakeVolume("a"); + auto* a_ptr = a.get(); + + REQUIRE(volumes.addVolume(std::move(a)) == a_ptr); + REQUIRE(volumes.addVolume(MakeVolume("a")) == nullptr); + } } From 43962df5edcb9e74f0dec32d31cd258bbb6ade43 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Mon, 4 Mar 2024 18:32:36 +0100 Subject: [PATCH 09/13] fix(Base): Only search `vList` once Fix #1495 --- fairroot/base/sim/FairModule.cxx | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index e98eab7a11..b134956338 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -250,19 +250,20 @@ void FairModule::ProcessNodes(TList* aList) } } -void FairModule::AddSensitiveVolume(TGeoVolume* v) +void FairModule::AddSensitiveVolume(TGeoVolume* vol) { - LOG(debug2) << "AddSensitiveVolume " << v->GetName(); - - // Only register volumes which are not already registered - // Otherwise the stepping will be slowed down - if (!vList->findObject(v->GetName())) { - auto addedVol = vList->addVolume(std::make_unique(v->GetName(), fNbOfVolumes++)); - addedVol->setModId(fModId); - addedVol->SetModule(this); - fAllSensitiveVolumes.push_back(addedVol); - fNbOfSensitiveVol++; + auto volName = vol->GetName(); + LOG(debug2) << "AddSensitiveVolume " << volName; + + auto addedVol = vList->addVolume(std::make_unique(volName, fNbOfVolumes)); + if (!addedVol) { + return; } + ++fNbOfVolumes; + addedVol->setModId(fModId); + addedVol->SetModule(this); + fAllSensitiveVolumes.push_back(addedVol); + ++fNbOfSensitiveVol; } FairVolume* FairModule::getFairVolume(FairGeoNode* fN) From 98aa8ba841264bc1ac23e78a1bc791ead834233e Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Sun, 26 May 2024 16:56:54 +0200 Subject: [PATCH 10/13] refactor(Base): Rewrite loop with `TRangeDynCast` for readability * Rename `aList` to `nodes`, see [S.2](https://github.com/FairRootGroup/FairRoot/blob/master/CONTRIBUTING.md#s2-prefer-variable-names-describing-what-they-contain) * Apply [CPPCG::ES.6](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es6-declare-names-in-for-statement-initializers-and-conditions-to-limit-scope) --- fairroot/base/sim/FairModule.cxx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index b134956338..923fb262f2 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -26,7 +26,7 @@ #include "FairVolumeList.h" // for FairVolumeList #include // for TBuffer, operator<<, etc -#include // for TIter +#include // for TIter, TRangeDynCast #include // for TDirectory::TContext #include // for TFile #include // for TGeoManager, gGeoManager @@ -198,7 +198,7 @@ void FairModule::SetGeometryFileName(TString fname, TString) fgeoName = ""; } -void FairModule::ProcessNodes(TList* aList) +void FairModule::ProcessNodes(TList* nodes) { if (FairMCApplicationState::kConstructGeometry != FairMCApplication::Instance()->GetState()) { LOG(fatal) << "Detected call to FairModule::ProcessNodes() \ @@ -211,14 +211,14 @@ void FairModule::ProcessNodes(TList* aList) vList = new FairVolumeList(); } - TListIter iter(aList); - FairGeoNode* node = nullptr; FairGeoNode* MotherNode = nullptr; FairRuntimeDb* rtdb = FairRun::Instance()->GetRuntimeDb(); FairGeoParSet* par = static_cast(rtdb->getContainer("FairGeoParSet")); TObjArray* fNodes = par->GetGeoNodes(); - while ((node = static_cast(iter.Next()))) { - + for (auto node : TRangeDynCast(nodes)) { + if (!node) { + continue; + } node->calcLabTransform(); MotherNode = node->getMotherNode(); auto nodeTruncName = node->getTruncName(); From fbc4fe6155ae19cfdfa38ed35521dbcf17d59cf1 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Sun, 26 May 2024 17:26:02 +0200 Subject: [PATCH 11/13] refactor(Base): `FairModule::ProcessNodes` further * [CPPCG::ES.5 Keep scopes small](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es5-keep-scopes-small) * [CPPCG::ES.11 Use `auto` to avoid redundant repetition of type names](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto) * Rename `fNodes` to `parNodes` - the `f`-prefix is reserved for member variables * Default to prefix increment operator for readibility, especially if return value is ignored * `FairGeoNode` is a `FairGeoVolume` - no `dynamic_cast` needed * Explicitely `std::ignore` return values --- fairroot/base/sim/FairModule.cxx | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index 923fb262f2..d353e82c63 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -36,6 +36,7 @@ #include // for TKey #include // for TObjArray #include // for TObject +#include // for TSeqCollection #include // for TSystem, gSystem #include @@ -47,6 +48,7 @@ #include // for strcmp, strlen #include #include +#include // for std::ignore thread_local std::vector FairModule::fAllSensitiveVolumes; @@ -211,19 +213,19 @@ void FairModule::ProcessNodes(TList* nodes) vList = new FairVolumeList(); } - FairGeoNode* MotherNode = nullptr; - FairRuntimeDb* rtdb = FairRun::Instance()->GetRuntimeDb(); - FairGeoParSet* par = static_cast(rtdb->getContainer("FairGeoParSet")); - TObjArray* fNodes = par->GetGeoNodes(); + auto rtdb = FairRun::Instance()->GetRuntimeDb(); + auto par = static_cast(rtdb->getContainer("FairGeoParSet")); + TSeqCollection* parNodes = par->GetGeoNodes(); for (auto node : TRangeDynCast(nodes)) { if (!node) { continue; } - node->calcLabTransform(); - MotherNode = node->getMotherNode(); + std::ignore = node->calcLabTransform(); + auto nodeTruncName = node->getTruncName(); auto volume = std::make_unique(nodeTruncName, fNbOfVolumes++); volume->setRealName(node->GetName()); + auto addedVol = vList->addVolume(std::move(volume)); if (!addedVol) { LOG(warn) << "Skipping further processing of FairGeoNode " << nodeTruncName @@ -233,19 +235,18 @@ void FairModule::ProcessNodes(TList* nodes) addedVol->setGeoNode(node); addedVol->setCopyNo(node->getCopyNo()); - if (MotherNode != 0) { + auto motherNode = node->getMotherNode(); + if (motherNode) { addedVol->setMotherId(node->getMCid()); - addedVol->setMotherCopyNo(MotherNode->getCopyNo()); + addedVol->setMotherCopyNo(motherNode->getCopyNo()); } - FairGeoVolume* aVol = nullptr; if (node->isSensitive() && fActive) { addedVol->setModId(fModId); addedVol->SetModule(this); fAllSensitiveVolumes.push_back(addedVol); - aVol = dynamic_cast(node); - fNodes->AddLast(aVol); - fNbOfSensitiveVol++; + ++fNbOfSensitiveVol; + parNodes->AddLast(node); } } } From e72aa4f7e6d11c8e506cc3bc6fa35e64da4d324e Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Sun, 26 May 2024 17:57:57 +0200 Subject: [PATCH 12/13] refactor(Base): Deduplicate code into `FairModule::RegisterSensitiveVolume()` * Improve maintainability --- fairroot/base/sim/FairModule.cxx | 18 ++++++++++-------- fairroot/base/sim/FairModule.h | 2 ++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index d353e82c63..b6b799238d 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -200,6 +200,14 @@ void FairModule::SetGeometryFileName(TString fname, TString) fgeoName = ""; } +void FairModule::RegisterSensitiveVolume(FairVolume& vol) +{ + vol.setModId(fModId); + vol.SetModule(this); + fAllSensitiveVolumes.push_back(&vol); + ++fNbOfSensitiveVol; +} + void FairModule::ProcessNodes(TList* nodes) { if (FairMCApplicationState::kConstructGeometry != FairMCApplication::Instance()->GetState()) { @@ -242,10 +250,7 @@ void FairModule::ProcessNodes(TList* nodes) } if (node->isSensitive() && fActive) { - addedVol->setModId(fModId); - addedVol->SetModule(this); - fAllSensitiveVolumes.push_back(addedVol); - ++fNbOfSensitiveVol; + RegisterSensitiveVolume(*addedVol); parNodes->AddLast(node); } } @@ -261,10 +266,7 @@ void FairModule::AddSensitiveVolume(TGeoVolume* vol) return; } ++fNbOfVolumes; - addedVol->setModId(fModId); - addedVol->SetModule(this); - fAllSensitiveVolumes.push_back(addedVol); - ++fNbOfSensitiveVol; + RegisterSensitiveVolume(*addedVol); } FairVolume* FairModule::getFairVolume(FairGeoNode* fN) diff --git a/fairroot/base/sim/FairModule.h b/fairroot/base/sim/FairModule.h index 7ca7b11ea4..8281892f83 100644 --- a/fairroot/base/sim/FairModule.h +++ b/fairroot/base/sim/FairModule.h @@ -158,6 +158,8 @@ class FairModule : public TNamed void ReAssignMediaId(); void swap(FairModule& other) throw(); + void RegisterSensitiveVolume(FairVolume&); + protected: FairModule(const FairModule&); FairModule& operator=(const FairModule&); From 135bd40d80a26b6c9afbb87ab1d963bc740f1140 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Sun, 26 May 2024 18:00:07 +0200 Subject: [PATCH 13/13] fix(Base): Only increment volume counter if volume is actually added --- fairroot/base/sim/FairModule.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fairroot/base/sim/FairModule.cxx b/fairroot/base/sim/FairModule.cxx index b6b799238d..acf2856efd 100644 --- a/fairroot/base/sim/FairModule.cxx +++ b/fairroot/base/sim/FairModule.cxx @@ -231,7 +231,7 @@ void FairModule::ProcessNodes(TList* nodes) std::ignore = node->calcLabTransform(); auto nodeTruncName = node->getTruncName(); - auto volume = std::make_unique(nodeTruncName, fNbOfVolumes++); + auto volume = std::make_unique(nodeTruncName, fNbOfVolumes); volume->setRealName(node->GetName()); auto addedVol = vList->addVolume(std::move(volume)); @@ -240,6 +240,7 @@ void FairModule::ProcessNodes(TList* nodes) << ". It is already present in the global volume list."; continue; } + ++fNbOfVolumes; addedVol->setGeoNode(node); addedVol->setCopyNo(node->getCopyNo());