From 1d7adb5a16302edd1067a46cf47fc09ef75c7f01 Mon Sep 17 00:00:00 2001
From: scott snyder <snyder@bnl.gov>
Date: Tue, 8 Dec 2020 22:40:33 +0100
Subject: [PATCH] TrkGeometry: Fix memory leaks
CompactBinnedArray maps bins in some coordinates to a vector of pointers
to objects. It does not own this objects.
BinnedMaterial uses CompactBinnedArray, with the object in question
being a pair<> object.
When a BinnedMaterial is constructed, it takes a vector of pointers
to these pairs. It does not take ownership of this pairs, but just
passes them to the CompactBinnedArray.
The callers of BinnedMaterial also do not take ownership of these pairs.
So they are presently leaked.
Further, BinnedMaterial can't just delete them, because these pairs
get shared between multiple BinnedMaterial instances.
We restructure like this.
BinnnedMaterial now gets a vector of pairs, which it saves as a member.
The CompactBinnedArray is then constructed to that it points at the
pairs in this vector. BinnedMaterial now effectively owns the pairs.
These are now duplicated in each BinnedMaterial, but that should be ok.
We also need to extend CompactBinnedArray so that we can give it
a new vector of object pointers when it is cloned.
cf ATLASRECTS-5831.
---
.../TrkGeometry/TrkGeometry/BinnedMaterial.h | 39 ++++-------
.../TrkGeometry/src/BinnedMaterial.cxx | 67 ++++++++++++++++---
2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/Tracking/TrkDetDescr/TrkGeometry/TrkGeometry/BinnedMaterial.h b/Tracking/TrkDetDescr/TrkGeometry/TrkGeometry/BinnedMaterial.h
index ab6c04884cdc..d7bcd1bc2cda 100644
--- a/Tracking/TrkDetDescr/TrkGeometry/TrkGeometry/BinnedMaterial.h
+++ b/Tracking/TrkDetDescr/TrkGeometry/TrkGeometry/BinnedMaterial.h
@@ -16,6 +16,7 @@
#include <iomanip>
#include <string>
#include <climits>
+#include <memory>
#include "TrkGeometry/Material.h"
#include "TrkDetDescrUtils/CompactBinnedArray.h"
#include "TrkDetDescrUtils/CompactBinnedArray1D.h"
@@ -38,9 +39,7 @@ namespace Trk {
public:
/** Default Constructor needed for POOL */
- BinnedMaterial():
- Material(),
- m_matBins(nullptr){}
+ BinnedMaterial() = default;
/** Constructor with arguments */
BinnedMaterial(float iX0,
@@ -55,37 +54,21 @@ namespace Trk {
/** Constructor with averaged material and binning in 1D*/
BinnedMaterial(const Material*& mat, BinUtility*& bu, const std::vector<size_t>& index,
- const std::vector< const IdentifiedMaterial*>& detailedMat);
+ const std::vector<IdentifiedMaterial>& detailedMat);
/** Constructor with averaged material and binning in 2D*/
BinnedMaterial(const Material*& mat, BinUtility*& bu, std::vector< Trk::BinUtility*>& bVec,
const std::vector<std::vector<size_t> >& index,
- const std::vector< const IdentifiedMaterial* >& detailedMat);
+ const std::vector<IdentifiedMaterial>& detailedMat);
/** Copy Constructor */
- BinnedMaterial(const BinnedMaterial& amc) :
- Material(amc),
- m_matBins(amc.m_matBins? amc.m_matBins->clone() : nullptr )
- {}
+ BinnedMaterial(const BinnedMaterial& amc);
- /** Desctructor - delete the composition if there */
- ~BinnedMaterial() { if (m_matBins) delete m_matBins; }
+ /** Destructor - delete the composition if there */
+ ~BinnedMaterial() = default;
/** Assignment operator */
- BinnedMaterial& operator=(const BinnedMaterial& amc) {
- if (this != &amc){
- X0 = amc.X0;
- L0 = amc.L0;
- A = amc.A;
- Z = amc.Z;
- rho = amc.rho;
- dEdX = amc.dEdX;
- zOaTr = amc.zOaTr;
- delete m_matBins;
- m_matBins = amc.m_matBins ? amc.m_matBins->clone() : nullptr;
- }
- return (*this);
- }
+ BinnedMaterial& operator=(const BinnedMaterial& amc);
/** access to layer bin utility */
const Trk::BinUtility* layerBinUtility(const Amg::Vector3D& position) const;
@@ -98,8 +81,10 @@ namespace Trk {
const IdentifiedMaterial* materialNext(const Amg::Vector3D& pos,const Amg::Vector3D& dir, bool layOnly ) const;
private:
- const CompactBinnedArray< const IdentifiedMaterial >* m_matBins;
-
+ std::vector<const Trk::IdentifiedMaterial* > ptrs() const;
+ std::vector<IdentifiedMaterial> m_matVec;
+ using binsPtr_t = std::unique_ptr<const CompactBinnedArray< const IdentifiedMaterial > >;
+ binsPtr_t m_matBins;
};
diff --git a/Tracking/TrkDetDescr/TrkGeometry/src/BinnedMaterial.cxx b/Tracking/TrkDetDescr/TrkGeometry/src/BinnedMaterial.cxx
index c72924044ad0..351094fa2cfa 100755
--- a/Tracking/TrkDetDescr/TrkGeometry/src/BinnedMaterial.cxx
+++ b/Tracking/TrkDetDescr/TrkGeometry/src/BinnedMaterial.cxx
@@ -6,27 +6,63 @@
/** Constructor with averaged material and binning in 1D*/
Trk::BinnedMaterial::BinnedMaterial(const Trk::Material*& mat, Trk::BinUtility*& bu, const std::vector<size_t>& index,
- const std::vector< const Trk::IdentifiedMaterial* >& detailedMat ) :
+ const std::vector<Trk::IdentifiedMaterial>& detailedMat ) :
Trk::Material(*mat),
- m_matBins(nullptr)
+ m_matVec (detailedMat),
+ m_matBins (std::make_unique<Trk::CompactBinnedArray1D<const Trk::IdentifiedMaterial> >(ptrs(),index,bu))
{
- Trk::CompactBinnedArray1D<const Trk::IdentifiedMaterial >* bm =
- new Trk::CompactBinnedArray1D<const Trk::IdentifiedMaterial >(detailedMat,index,bu);
- m_matBins = bm;
}
/** Constructor with averaged material and binning in 2D*/
Trk::BinnedMaterial::BinnedMaterial(const Trk::Material*& mat, Trk::BinUtility*& bu, std::vector< Trk::BinUtility*>& bVec,
const std::vector<std::vector<size_t> >& index,
- const std::vector<const Trk::IdentifiedMaterial* >& detailedMat ) :
+ const std::vector<Trk::IdentifiedMaterial>& detailedMat ) :
Trk::Material(*mat),
- m_matBins(nullptr)
+ m_matVec (detailedMat),
+ m_matBins (std::make_unique<Trk::CompactBinnedArray2D<const Trk::IdentifiedMaterial> >(ptrs(),index,bu,bVec))
{
- Trk::CompactBinnedArray2D<const Trk::IdentifiedMaterial >* bm =
- new Trk::CompactBinnedArray2D<const Trk::IdentifiedMaterial >(detailedMat,index,bu,bVec);
- m_matBins = bm;
}
+Trk::BinnedMaterial::BinnedMaterial(const BinnedMaterial& amc) :
+ Material(amc),
+ m_matVec (amc.m_matVec)
+{
+ if (amc.m_matBins) {
+ if (!m_matVec.empty()) {
+ m_matBins = binsPtr_t (amc.m_matBins->clone(ptrs()));
+ }
+ else {
+ m_matBins = binsPtr_t (amc.m_matBins->clone());
+ }
+ }
+}
+
+/** Assignment operator */
+Trk::BinnedMaterial&
+Trk::BinnedMaterial::operator=(const BinnedMaterial& amc)
+{
+ if (this != &amc){
+ X0 = amc.X0;
+ L0 = amc.L0;
+ A = amc.A;
+ Z = amc.Z;
+ rho = amc.rho;
+ dEdX = amc.dEdX;
+ zOaTr = amc.zOaTr;
+ m_matVec = amc.m_matVec;
+ m_matBins.reset();
+ if (amc.m_matBins) {
+ if (!m_matVec.empty()) {
+ m_matBins = binsPtr_t (amc.m_matBins->clone(ptrs()));
+ }
+ else {
+ m_matBins = binsPtr_t (amc.m_matBins->clone());
+ }
+ }
+ }
+ return (*this);
+}
+
/** access to binned material */
const Trk::IdentifiedMaterial* Trk::BinnedMaterial::material(const Amg::Vector3D& position ) const
{
@@ -41,3 +77,14 @@ const Trk::IdentifiedMaterial* Trk::BinnedMaterial::materialNext(const Amg::Vect
return mat ;
}
+
+std::vector<const Trk::IdentifiedMaterial* >
+Trk::BinnedMaterial::ptrs() const
+{
+ std::vector<const Trk::IdentifiedMaterial* > p;
+ p.reserve (m_matVec.size());
+ for (const Trk::IdentifiedMaterial& m : m_matVec) {
+ p.push_back (&m);
+ }
+ return p;
+}
--
GitLab