Merge branch 'GeoGraphNodeInPtr' into 'main'

GeoPhysVol / GeoFullPhysVol - Pointer managment clean up

See merge request !228

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoFullPhysVol.h

 /*
-  Copyright (C) 2002-2022 CERN for the benefit of the ATLAS collaboration
+  Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
 */
 
 #ifndef GEOMODELKERNEL_GEOFULLPHYSVOL_H
@@ -72,16 +72,14 @@ class GeoFullPhysVol final : public GeoVFullPhysVol
   virtual const GeoGraphNode * const *findChildNode(const GeoGraphNode *n) const override;
 
   protected:
-  virtual ~GeoFullPhysVol() override;
+  virtual ~GeoFullPhysVol();
 
   private:
-  GeoFullPhysVol(const GeoFullPhysVol &right);
-  GeoFullPhysVol & operator=(const GeoFullPhysVol &right);
-
+ 
   /// Hold the list of children.
-  std::vector<const GeoGraphNode *> m_daughters;
+  std::vector<GeoIntrusivePtr<GeoGraphNode>> m_daughters{};
   
-  const GeoFullPhysVol* m_cloneOrigin;
+  const GeoFullPhysVol* m_cloneOrigin{nullptr};
 };
 
 #endif

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoGraphNode.h

 /*
-  Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+  Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
 */
 
 #ifndef GEOMODELKERNEL_GEOGRAPHNODE_H
@@ -25,10 +25,9 @@
 class GeoNodeAction;
 class GeoVPhysVol;
 
-class GeoGraphNode : public RCBase
-{
+class GeoGraphNode : public RCBase {
  public:
-  GeoGraphNode ();
+  GeoGraphNode () = default;
   
   //	Executes a GeoNodeAction.
   virtual void exec (GeoNodeAction *action) const;
@@ -42,11 +41,8 @@ class GeoGraphNode : public RCBase
   virtual void dockTo (GeoVPhysVol* );
   
  protected:
-  virtual ~GeoGraphNode();
+  virtual ~GeoGraphNode() = default;
   
- private:
-  GeoGraphNode(const GeoGraphNode &right);
-  GeoGraphNode & operator=(const GeoGraphNode &right);
 };
 
 #endif

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoPhysVol.h

 /*
-  Copyright (C) 2002-2022 CERN for the benefit of the ATLAS collaboration
+  Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
 */
 
 #ifndef GEOMODELKERNEL_GEOPHYSVOL_H
@@ -28,14 +28,10 @@
 #include <mutex>
 
 
-class GeoPhysVol : public GeoVPhysVol
-{
+class GeoPhysVol : public GeoVPhysVol {
  public:
   GeoPhysVol(const GeoLogVol* LogVol);
 
-  GeoPhysVol(const GeoPhysVol &right) = delete;
-  GeoPhysVol & operator=(const GeoPhysVol &right) = delete;
-
   /// Adds a Graph Node to the Geometry Graph
   virtual void add(GeoGraphNode* graphNode) override final;
 
@@ -72,10 +68,10 @@ class GeoPhysVol : public GeoVPhysVol
   virtual const GeoGraphNode * const *findChildNode(const GeoGraphNode *n) const override final;
 
  protected:
-  virtual ~GeoPhysVol() override;
+    virtual ~GeoPhysVol() = default;
 
  private:
-  std::vector<const GeoGraphNode*> m_daughters;
+  std::vector<GeoIntrusivePtr<GeoGraphNode>> m_daughters{};
   mutable std::mutex m_muxVec;
 };
 

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoVFullPhysVol.h

@@ -8,6 +8,7 @@
 #include "GeoModelKernel/GeoVPhysVol.h"
 #include "GeoModelKernel/GeoAbsPositionInfo.h"
 #include <mutex>
+#include <memory>
 
 class GeoVAlignmentStore;
 
@@ -17,9 +18,6 @@ class GeoVFullPhysVol : public GeoVPhysVol
  public:
   GeoVFullPhysVol(const GeoLogVol* logVol);
 
-  GeoVFullPhysVol(const GeoVFullPhysVol &right) = delete;
-  GeoVFullPhysVol & operator=(const GeoVFullPhysVol &right) = delete;
-
   /// Returns the (default) absolute transform of the volume.
   /// 1. When store=nullptr. This is considered a "serial case" when
   /// the cached (default) absolute transform is kept as a data member of the
@@ -50,7 +48,7 @@ class GeoVFullPhysVol : public GeoVPhysVol
   unsigned int getId() const;
 
  protected:
-  virtual ~GeoVFullPhysVol() override;
+  virtual ~GeoVFullPhysVol() = default;
 
   /// Mutex serving dual purpose:
   ///  1. To protect the absolute position info
@@ -59,13 +57,13 @@ class GeoVFullPhysVol : public GeoVPhysVol
 
  private:
   /// The absolute name of this volume.
-  mutable std::string m_absName;
+  mutable std::string m_absName{};
 
   /// An identifier. This is locally cached in a full physical volume.
-  mutable Query<int> *m_id;
+  mutable std::unique_ptr<Query<int>> m_id{nullptr};
 
   /// Information on the where this volume is, by default and after alignment corrections.      
-  mutable GeoAbsPositionInfo *m_absPosInfo;
+  mutable std::unique_ptr<GeoAbsPositionInfo> m_absPosInfo{nullptr};
 
 };
 

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoVPhysVol.h

@@ -27,8 +27,6 @@ class GeoVPhysVol : public GeoGraphNode
  public:
   GeoVPhysVol(const GeoLogVol* LogVol);
   
-  GeoVPhysVol(const GeoVPhysVol &right) = delete;
-  GeoVPhysVol & operator=(const GeoVPhysVol &right) = delete;
 
   /// Returns true if the physical volume is accessed by more than one parent.
   /// Should check this before trusting the parent pointer.

GeoModelCore/GeoModelKernel/src/GeoFullPhysVol.cxx

@@ -12,14 +12,9 @@
 #include <algorithm>
 
 GeoFullPhysVol::GeoFullPhysVol (const GeoLogVol* LogVol)
-  : GeoVFullPhysVol(LogVol)
-  , m_cloneOrigin(nullptr)
-{
-}
+  : GeoVFullPhysVol(LogVol){ }
 
-GeoFullPhysVol::~GeoFullPhysVol()
-{
-  for(const GeoGraphNode* daughter : m_daughters) daughter->unref();
+GeoFullPhysVol::~GeoFullPhysVol() {
   if(m_cloneOrigin && m_cloneOrigin!=this) m_cloneOrigin->unref();
 }
 
@@ -28,7 +23,6 @@ void GeoFullPhysVol::add(GeoGraphNode* graphNode)
   if(m_cloneOrigin) throw std::runtime_error("Attempt to modify contents of a cloned FPV");
   std::scoped_lock<std::mutex> guard(m_mutex);
   m_daughters.push_back(graphNode);
-  graphNode->ref();
   graphNode->dockTo(this);
 }
 
@@ -239,19 +233,17 @@ unsigned int GeoFullPhysVol::getNChildNodes() const
   return m_daughters.size();
 }
 
-const GeoGraphNode * const * GeoFullPhysVol::getChildNode(unsigned int i) const 
-{
+const GeoGraphNode * const * GeoFullPhysVol::getChildNode(unsigned int i) const {
   std::scoped_lock<std::mutex> guard(m_mutex);
-  return &(m_daughters[i]);
+  return m_daughters[i];
 }
 const GeoGraphNode * const * GeoFullPhysVol::findChildNode(const GeoGraphNode * n) const 
 {
   std::scoped_lock<std::mutex> guard(m_mutex);
-  std::vector<const GeoGraphNode *>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
+  std::vector<GeoIntrusivePtr<GeoGraphNode>>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
   if (i==m_daughters.end()) {
     return nullptr;
   }
-  else {
-    return &*i;
-  }
+  return (*i);
+  
 }

GeoModelCore/GeoModelKernel/src/GeoGraphNode.cxx

 /*
-  Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+  Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
 */
 
 #include "GeoModelKernel/GeoGraphNode.h"
 #include "GeoModelKernel/GeoNodeAction.h"
 
-GeoGraphNode::GeoGraphNode ()
-{
-}
-
-GeoGraphNode::~GeoGraphNode()
-{
-}
 
 void GeoGraphNode::exec (GeoNodeAction *action) const
 {

GeoModelCore/GeoModelKernel/src/GeoPhysVol.cxx

 /*
-  Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+  Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
 */
 
 #include "GeoModelKernel/GeoPhysVol.h"
@@ -17,17 +17,11 @@ GeoPhysVol::GeoPhysVol(const GeoLogVol* LogVol)
 {
 }
 
-GeoPhysVol::~GeoPhysVol()
-{
-  std::scoped_lock<std::mutex> lk(m_muxVec);
-  for(const GeoGraphNode* daughter : m_daughters) daughter->unref();
-}
 
 void GeoPhysVol::add(GeoGraphNode* graphNode)
 {
   std::scoped_lock<std::mutex> lk(m_muxVec);
   m_daughters.push_back(graphNode);
-  graphNode->ref();
   graphNode->dockTo(this);
 }
 
@@ -199,20 +193,17 @@ unsigned int GeoPhysVol::getNChildNodes() const
   return m_daughters.size();
 }
 
-const GeoGraphNode * const * GeoPhysVol::getChildNode(unsigned int i) const 
-{
+const GeoGraphNode * const * GeoPhysVol::getChildNode(unsigned int i) const {
   std::scoped_lock<std::mutex> lk(m_muxVec);
-  return &(m_daughters[i]);
+  return m_daughters[i];
 }
 
-const GeoGraphNode * const * GeoPhysVol::findChildNode(const GeoGraphNode * n) const 
-{
+const GeoGraphNode * const * GeoPhysVol::findChildNode(const GeoGraphNode * n) const {
   std::scoped_lock<std::mutex> lk(m_muxVec);
-  std::vector<const GeoGraphNode *>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
+  std::vector<GeoIntrusivePtr<GeoGraphNode>>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
   if (i==m_daughters.end()) {
     return nullptr;
   }
-  else {
-    return &*i;
-  }
+  return (*i);
+ 
 }

GeoModelCore/GeoModelKernel/src/GeoVFullPhysVol.cxx

@@ -7,17 +7,8 @@
 #include <string>
 
 GeoVFullPhysVol::GeoVFullPhysVol(const GeoLogVol* logVol)
-  : GeoVPhysVol(logVol)
-  , m_id(nullptr)
-  , m_absPosInfo(nullptr)
-{
-}
+  : GeoVPhysVol(logVol){}
 
-GeoVFullPhysVol::~GeoVFullPhysVol()
-{
-  delete m_absPosInfo;
-  delete m_id;
-}
 
 const GeoTrf::Transform3D & GeoVFullPhysVol::getAbsoluteTransform(GeoVAlignmentStore* store) const
 {
@@ -33,15 +24,14 @@ const GeoTrf::Transform3D & GeoVFullPhysVol::getAbsoluteTransform(GeoVAlignmentS
 
   std::scoped_lock<std::mutex> guard(m_mutex);
   
-  if(store==nullptr && !m_absPosInfo) m_absPosInfo = new GeoAbsPositionInfo();
+  if(!store && !m_absPosInfo) m_absPosInfo = std::make_unique<GeoAbsPositionInfo>();
 
   //
   // Check the cache first. If not empty, then return the cached value
   //
-  if(store==nullptr){
+  if(!store) {
     if(m_absPosInfo->getAbsTransform()) return *m_absPosInfo->getAbsTransform();
-  }
-  else {
+  } else {
     const GeoTrf::Transform3D* storedPos = store->getAbsPosition(this);
     if(storedPos!=nullptr) return *storedPos;
   }
@@ -95,8 +85,7 @@ const GeoTrf::Transform3D& GeoVFullPhysVol::getCachedAbsoluteTransform(const Geo
 void GeoVFullPhysVol::clearPositionInfo() const
 {
   std::scoped_lock<std::mutex> guard(m_mutex);
-  delete m_absPosInfo;
-  m_absPosInfo = nullptr;
+  m_absPosInfo.reset();
 }
 
 const GeoTrf::Transform3D& GeoVFullPhysVol::getDefAbsoluteTransform(GeoVAlignmentStore* store) const
@@ -112,15 +101,14 @@ const GeoTrf::Transform3D& GeoVFullPhysVol::getDefAbsoluteTransform(GeoVAlignmen
 
   std::scoped_lock<std::mutex> guard(m_mutex);
   
-  if(store==nullptr && !m_absPosInfo) m_absPosInfo = new GeoAbsPositionInfo();
+  if(!store && !m_absPosInfo) m_absPosInfo = std::make_unique<GeoAbsPositionInfo>();
 
   //
   // Check the cache first. If not empty, then return the cached value
   //
-  if(store==nullptr){
+  if(!store){
     if(m_absPosInfo->getDefAbsTransform()) return *m_absPosInfo->getDefAbsTransform();
-  }
-  else {
+  } else {
     const GeoTrf::Transform3D* storedPos = store->getDefAbsPosition(this);
     if(storedPos!=nullptr) return *storedPos;
   }
@@ -219,7 +207,7 @@ unsigned int GeoVFullPhysVol::getId () const
   //                                                                                                //     
   //------------------------------------------------------------------------------------------------//     
 
-  if(m_id==nullptr) {
+  if(!m_id) {
     if(isShared()) throw std::runtime_error(errorMessage);
 
     //     
@@ -235,7 +223,7 @@ unsigned int GeoVFullPhysVol::getId () const
     }
 
     int index = parent->indexOf(child);
-    m_id = new Query<int>(parent->getIdOfChildVol(index));
+    m_id = std::make_unique<Query<int>>(parent->getIdOfChildVol(index));
   }
   return *m_id;
 }