Fix #1495

fairroot/base/sim/FairModule.cxx

@@ -26,7 +26,7 @@
 #include "FairVolumeList.h"   // for FairVolumeList
 
 #include <TBuffer.h>           // for TBuffer, operator<<, etc
-#include <TCollection.h>       // for TIter
+#include <TCollection.h>       // for TIter, TRangeDynCast
 #include <TDirectory.h>        // for TDirectory::TContext
 #include <TFile.h>             // for TFile
 #include <TGeoManager.h>       // for TGeoManager, gGeoManager
@@ -36,7 +36,9 @@
 #include <TKey.h>              // for TKey
 #include <TObjArray.h>         // for TObjArray
 #include <TObject.h>           // for TObject
+#include <TSeqCollection.h>    // for TSeqCollection
 #include <TSystem.h>           // for TSystem, gSystem
+#include <fairlogger/Logger.h>
 
 #ifdef ROOT_HAS_GDML
 #include <TGDMLParse.h>
@@ -46,6 +48,7 @@
 #include <cstring>   // for strcmp, strlen
 #include <map>
 #include <memory>
+#include <tuple>   // for std::ignore
 
 thread_local std::vector<FairVolume*> FairModule::fAllSensitiveVolumes;
 
@@ -197,7 +200,15 @@ void FairModule::SetGeometryFileName(TString fname, TString)
     fgeoName = "";
 }
 
-void FairModule::ProcessNodes(TList* aList)
+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()) {
         LOG(fatal) << "Detected call to FairModule::ProcessNodes() \
@@ -210,55 +221,53 @@ void FairModule::ProcessNodes(TList* aList)
         vList = new FairVolumeList();
     }
 
-    TListIter iter(aList);
-    FairGeoNode* node = nullptr;
-    FairGeoNode* MotherNode = nullptr;
-    FairVolume* volume = nullptr;
-    FairRuntimeDb* rtdb = FairRun::Instance()->GetRuntimeDb();
-    FairGeoParSet* par = static_cast<FairGeoParSet*>(rtdb->getContainer("FairGeoParSet"));
-    TObjArray* fNodes = par->GetGeoNodes();
-    while ((node = static_cast<FairGeoNode*>(iter.Next()))) {
-
-        node->calcLabTransform();
-        MotherNode = node->getMotherNode();
-        volume = new FairVolume(node->getTruncName(), fNbOfVolumes++);
+    auto rtdb = FairRun::Instance()->GetRuntimeDb();
+    auto par = static_cast<FairGeoParSet*>(rtdb->getContainer("FairGeoParSet"));
+    TSeqCollection* parNodes = par->GetGeoNodes();
+    for (auto node : TRangeDynCast<FairGeoNode>(nodes)) {
+        if (!node) {
+            continue;
+        }
+        std::ignore = node->calcLabTransform();
+
+        auto nodeTruncName = node->getTruncName();
+        auto volume = std::make_unique<FairVolume>(nodeTruncName, fNbOfVolumes);
         volume->setRealName(node->GetName());
-        vList->addVolume(volume);
-        volume->setGeoNode(node);
-        volume->setCopyNo(node->getCopyNo());
 
-        if (MotherNode != 0) {
-            volume->setMotherId(node->getMCid());
-            volume->setMotherCopyNo(MotherNode->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;
+        }
+        ++fNbOfVolumes;
+        addedVol->setGeoNode(node);
+        addedVol->setCopyNo(node->getCopyNo());
+
+        auto motherNode = node->getMotherNode();
+        if (motherNode) {
+            addedVol->setMotherId(node->getMCid());
+            addedVol->setMotherCopyNo(motherNode->getCopyNo());
         }
-        FairGeoVolume* aVol = nullptr;
 
         if (node->isSensitive() && fActive) {
-            volume->setModId(fModId);
-            volume->SetModule(this);
-            fAllSensitiveVolumes.push_back(volume);
-            aVol = dynamic_cast<FairGeoVolume*>(node);
-            fNodes->AddLast(aVol);
-            fNbOfSensitiveVol++;
+            RegisterSensitiveVolume(*addedVol);
+            parNodes->AddLast(node);
         }
     }
 }
 
-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())) {
-        FairVolume* volume = nullptr;
-        volume = new FairVolume(v->GetName(), fNbOfVolumes++);
-        vList->addVolume(volume);
-        volume->setModId(fModId);
-        volume->SetModule(this);
-        fAllSensitiveVolumes.push_back(volume);
-        fNbOfSensitiveVol++;
+    auto volName = vol->GetName();
+    LOG(debug2) << "AddSensitiveVolume " << volName;
+
+    auto addedVol = vList->addVolume(std::make_unique<FairVolume>(volName, fNbOfVolumes));
+    if (!addedVol) {
+        return;
     }
+    ++fNbOfVolumes;
+    RegisterSensitiveVolume(*addedVol);
 }
 
 FairVolume* FairModule::getFairVolume(FairGeoNode* fN)

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&);

fairroot/base/sim/FairVolumeList.cxx

@@ -12,68 +12,35 @@
 
 #include "FairVolumeList.h"
 
-#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)
+FairVolume* FairVolumeList::getVolume(const TString& name)
 {
+    auto 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<FairVolume*>(obj);
 }
 
-Int_t FairVolumeList::getVolumeId(TString* name)
+Int_t FairVolumeList::getVolumeId(const TString& name)
 {
-    FairVolume* vol = getVolume(name);
+    auto vol = getVolume(name);
 
-    if (vol) {
-        return vol->getVolumeId();
-    } else {
-        return -111;
-    }
+    return vol ? vol->getVolumeId() : fgkNotFound;
 }
 
-FairVolume* FairVolumeList::findObject(TString name)
+FairVolume* FairVolumeList::addVolume(std::unique_ptr<FairVolume> vol)
 {
-    FairVolume* obj = nullptr;
+    auto vol_found = findObject(vol->GetName());
 
-    for (int i = 0; i < fData->GetEntriesFast(); i++) {
-        obj = static_cast<FairVolume*>(fData->At(i));
-        if (obj) {
-            if (obj->GetName() == name) {
-                return obj;
-            }
-        }
+    if (vol_found) {
+        LOG(error) << "FairVolumeList element: " << vol->GetName() << " VolId : " << vol->getVolumeId()
+                   << " already defined " << vol_found->getVolumeId();
+        return nullptr;
     }
 
-    return nullptr;
-}
-
-void FairVolumeList::addVolume(FairVolume* elem)
-{
-    FairVolume* v = findObject(elem->GetName());
-
-    if (v) {
-        LOG(error) << "FairVolumeList element: " << elem->GetName() << " VolId : " << elem->getVolumeId()
-                   << " already defined " << v->getVolumeId();
-    } else {
-        fData->Add(elem);
-    }
+    auto vol_added = vol.get();
+    fData.Add(vol.release());
+    return vol_added;
 }

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,             *
@@ -14,6 +14,8 @@
 #include <TObjArray.h>   // for TObjArray
 #include <TObject.h>     // for TObject
 #include <TString.h>     // for TString
+#include <cassert>       // for assert
+#include <memory>        // for std::unique_ptr
 
 /**
  * This Object is only used for internal book keeping!
@@ -25,24 +27,39 @@
 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);
+    static constexpr Int_t fgkNotFound = -111;
 
-    FairVolume* findObject(TString name);
-    void addVolume(FairVolume* elem);
+    FairVolume* getVolume(const TString& name);
+    [[deprecated]] FairVolume* getVolume(TString* name)
+    {
+        assert(name);
+        return getVolume(*name);
+    }
 
-    Int_t getEntries() { return fData->GetEntries(); }
-    FairVolume* At(Int_t pos) { return (dynamic_cast<FairVolume*>(fData->At(pos))); }
+    Int_t getVolumeId(const TString& name);
+    [[deprecated]] Int_t getVolumeId(TString* name)
+    {
+        assert(name);
+        return getVolumeId(*name);
+    }
 
-    ClassDefOverride(FairVolumeList, 1);
+    FairVolume* findObject(const TString& name) { return static_cast<FairVolume*>(fData.FindObject(name.Data())); }
+
+    [[nodiscard]] FairVolume* addVolume(std::unique_ptr<FairVolume> vol);
+    [[deprecated]] FairVolume* addVolume(FairVolume* elem) { return addVolume(std::unique_ptr<FairVolume>(elem)); }
+
+    Int_t getEntries() { return fData.GetEntries(); }
+    FairVolume* At(Int_t pos) { return (dynamic_cast<FairVolume*>(fData.At(pos))); }
+
+    ClassDefOverride(FairVolumeList, 0);
 };
 
 #endif   // FAIR_VOLUMELIST_H

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
 )