Skip to content
Snippets Groups Projects
Commit 661778e1 authored by Johannes Junggeburth's avatar Johannes Junggeburth :dog2:
Browse files

Merge branch 'GeoIntrusiveUnitTest' into 'main'

Fix random pointer deletion

Closes atlas/geomodelatlas/GeoModelATLAS#13

See merge request !232
parents 3a7e347c 954ca666
No related branches found
No related tags found
1 merge request!232Fix random pointer deletion
Pipeline #6622864 passed with warnings
...@@ -45,3 +45,10 @@ install(TARGETS GeoModelKernel ...@@ -45,3 +45,10 @@ install(TARGETS GeoModelKernel
install( FILES ${HEADERS} install( FILES ${HEADERS}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/GeoModelKernel DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/GeoModelKernel
COMPONENT Development ) COMPONENT Development )
add_executable(testGeoIntrusivePtr tests/testGeoIntrusivePtr.cxx)
target_link_libraries( testGeoIntrusivePtr GeoModelKernel)
add_test(NAME testGeoIntrusivePtr
COMMAND testGeoIntrusivePtr)
\ No newline at end of file
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#define GEOMODELKERNEL_GeoIntrusivePtr_H #define GEOMODELKERNEL_GeoIntrusivePtr_H
#include <GeoModelKernel/RCBase.h> #include <GeoModelKernel/RCBase.h>
#include <type_traits>
#include <utility> #include <utility>
...@@ -14,42 +15,73 @@ class GeoIntrusivePtr{ ...@@ -14,42 +15,73 @@ class GeoIntrusivePtr{
public: public:
template <typename GeoTypeGrp> friend class GeoIntrusivePtr; template <typename GeoTypeGrp> friend class GeoIntrusivePtr;
GeoIntrusivePtr() = default; explicit GeoIntrusivePtr() noexcept = default;
// Standard constructor taking a bare pointer // Standard constructor taking a bare pointer
GeoIntrusivePtr(GeoType* obj): GeoIntrusivePtr(GeoType* obj) noexcept:
m_ptr{obj} { m_ptr{obj} {
if (m_ptr) obj->ref(); if (m_ptr) obj->ref();
} }
/// Copy constructor /// Copy constructor
template <class GeoTypeGrp> GeoIntrusivePtr(const GeoIntrusivePtr& other) noexcept:
GeoIntrusivePtr(const GeoIntrusivePtr<GeoTypeGrp>& other):
GeoIntrusivePtr{other.get()} {} GeoIntrusivePtr{other.get()} {}
/// Copy constructor for derived types
template <typename GeoTypeGrp,
typename = typename std::enable_if<!std::is_same<GeoType,GeoTypeGrp>::value, bool>>
GeoIntrusivePtr(const GeoIntrusivePtr<GeoTypeGrp>& other) noexcept:
GeoIntrusivePtr{other.get()} {}
/// Move constructor
explicit GeoIntrusivePtr(GeoIntrusivePtr&& other) noexcept:
m_ptr{other.m_ptr} {
other.m_ptr = nullptr;
}
/// Move constructor for derived types
template <typename GeoTypeGrp,
typename = typename std::enable_if<!std::is_same<GeoType,GeoTypeGrp>::value, bool>>
GeoIntrusivePtr(GeoIntrusivePtr<GeoTypeGrp>&& other) noexcept:
m_ptr{other.m_ptr} {
other.m_ptr = nullptr;
}
/// Assignment operator /// Assignment operator
template <class GeoTypeGrp> GeoIntrusivePtr& operator=(const GeoIntrusivePtr& other) noexcept {
GeoIntrusivePtr& operator=(const GeoIntrusivePtr<GeoTypeGrp>& other) { reset(other.get());
if (m_ptr != other.get()) { return *this;
if(m_ptr) m_ptr->unref(); }
GeoIntrusivePtr& operator=(GeoType* other) noexcept {
reset(other);
return *this;
}
/// Move assignment operator
GeoIntrusivePtr& operator=(GeoIntrusivePtr&& other) {
if (m_ptr && m_ptr == other.get()) {
m_ptr->unref();
} else {
m_ptr = other.get(); m_ptr = other.get();
if(m_ptr) m_ptr->ref();
} }
other.m_ptr = nullptr;
return *this; return *this;
} }
/// Move constructor template <typename GeoTypeGrp,
template <class GeoTypeGrp> typename = typename std::enable_if<!std::is_same<GeoType,GeoTypeGrp>::value, bool>>
GeoIntrusivePtr(GeoIntrusivePtr<GeoTypeGrp>&& other): GeoIntrusivePtr& operator=(GeoIntrusivePtr<GeoTypeGrp>&& other) {
m_ptr{std::move(other.m_ptr)} { other.m_ptr = nullptr;} if (m_ptr && m_ptr == other.get()) {
/// Move assignment operator
template <class GeoTypeGrp>
GeoIntrusivePtr& operator=(GeoIntrusivePtr<GeoTypeGrp>&& other) {
if (m_ptr != other.get()) {
if(m_ptr) m_ptr->unref();
m_ptr = std::move(other.m_ptr);
} else if (m_ptr) {
m_ptr->unref(); m_ptr->unref();
} else {
m_ptr = other.get();
} }
other.m_ptr = nullptr; other.m_ptr = nullptr;
return *this; return *this;
} }
/// Reset the pointer
void reset(GeoType* ptr = nullptr) {
if (m_ptr == ptr) return;
if (m_ptr) m_ptr->unref();
m_ptr = ptr;
if (m_ptr) m_ptr->ref();
}
/// Destructor /// Destructor
~GeoIntrusivePtr() { ~GeoIntrusivePtr() {
if (m_ptr) m_ptr->unref(); if (m_ptr) m_ptr->unref();
...@@ -69,10 +101,10 @@ class GeoIntrusivePtr{ ...@@ -69,10 +101,10 @@ class GeoIntrusivePtr{
/// Invalidity operator /// Invalidity operator
bool operator!() const { return !m_ptr; } bool operator!() const { return !m_ptr; }
/// Comparison operator /// Comparison operator
template <class GeoTypeGrp> // template <class GeoTypeGrp>
bool operator==(const GeoIntrusivePtr<GeoTypeGrp>& other) const { // bool operator==(const GeoIntrusivePtr<GeoTypeGrp>& other) const {
return m_ptr == other.m_ptr; // return m_ptr == other.m_ptr;
} // }
bool operator==(GeoType* other) const { bool operator==(GeoType* other) const {
return m_ptr == other; return m_ptr == other;
} }
...@@ -84,4 +116,4 @@ class GeoIntrusivePtr{ ...@@ -84,4 +116,4 @@ class GeoIntrusivePtr{
GeoType* m_ptr{nullptr}; GeoType* m_ptr{nullptr};
}; };
#endif #endif
\ No newline at end of file
...@@ -57,7 +57,7 @@ class GeoSerialTransformer : public GeoGraphNode ...@@ -57,7 +57,7 @@ class GeoSerialTransformer : public GeoGraphNode
std::unique_ptr<const GeoXF::Function> m_function{}; std::unique_ptr<const GeoXF::Function> m_function{};
// The physical volume to be multiply placed. // The physical volume to be multiply placed.
GeoIntrusivePtr<const GeoVPhysVol>m_physVol{}; GeoIntrusivePtr<const GeoVPhysVol> m_physVol{};
}; };
#endif #endif
...@@ -6,12 +6,6 @@ ...@@ -6,12 +6,6 @@
#define GEOMODELKERNEL_GEOVPHYSVOL_H #define GEOMODELKERNEL_GEOVPHYSVOL_H
#include "GeoModelKernel/GeoIntrusivePtr.h" #include "GeoModelKernel/GeoIntrusivePtr.h"
class GeoVPhysVol;
using PVLink = GeoIntrusivePtr<GeoVPhysVol>;
using GeoPVLink = PVLink;
using PVConstLink = GeoIntrusivePtr<const GeoVPhysVol>;
using GeoPVConstLink = PVConstLink;
#include "GeoModelKernel/GeoDefinitions.h" #include "GeoModelKernel/GeoDefinitions.h"
#include "GeoModelKernel/Query.h" #include "GeoModelKernel/Query.h"
#include <string> #include <string>
...@@ -22,10 +16,12 @@ using GeoPVConstLink = PVConstLink; ...@@ -22,10 +16,12 @@ using GeoPVConstLink = PVConstLink;
class GeoVolumeAction; class GeoVolumeAction;
class GeoVAlignmentStore; class GeoVAlignmentStore;
class GeoVPhysVol : public GeoGraphNode class GeoVPhysVol : public GeoGraphNode {
{
public: public:
GeoVPhysVol(const GeoLogVol* LogVol); using PVLink = GeoIntrusivePtr<GeoVPhysVol>;
using PVConstLink = GeoIntrusivePtr<const GeoVPhysVol>;
GeoVPhysVol(const GeoLogVol* LogVol);
/// Returns true if the physical volume is accessed by more than one parent. /// Returns true if the physical volume is accessed by more than one parent.
...@@ -102,5 +98,10 @@ class GeoVPhysVol : public GeoGraphNode ...@@ -102,5 +98,10 @@ class GeoVPhysVol : public GeoGraphNode
GeoIntrusivePtr<const GeoLogVol> m_logVol{}; GeoIntrusivePtr<const GeoLogVol> m_logVol{};
}; };
using PVLink = GeoVPhysVol::PVLink;
using PVConstLink = GeoVPhysVol::PVConstLink;
using GeoPVLink = PVLink;
using GeoPVConstLink = PVConstLink;
#endif #endif
...@@ -26,18 +26,20 @@ class RCBase { ...@@ -26,18 +26,20 @@ class RCBase {
RCBase() = default; RCBase() = default;
// Increase the reference count // Increase the reference count
void ref() const { void ref() const noexcept {
++m_count; ++m_count;
} }
// Decreases the reference count. When the reference count // Decreases the reference count. When the reference count
// falls to zero, the object deletes itself. // falls to zero, the object deletes itself.
void unref () const { void unref () const noexcept{
if (--m_count == 0) delete this; if (--m_count == 0) {
delete this;
}
} }
// Return the reference count. // Return the reference count.
unsigned int refCount () const { unsigned int refCount () const noexcept {
return m_count.load(); return m_count.load();
} }
......
...@@ -22,7 +22,8 @@ void GeoFullPhysVol::add(GeoGraphNode* graphNode) ...@@ -22,7 +22,8 @@ void GeoFullPhysVol::add(GeoGraphNode* graphNode)
{ {
if(m_cloneOrigin) throw std::runtime_error("Attempt to modify contents of a cloned FPV"); if(m_cloneOrigin) throw std::runtime_error("Attempt to modify contents of a cloned FPV");
std::scoped_lock<std::mutex> guard(m_mutex); std::scoped_lock<std::mutex> guard(m_mutex);
m_daughters.push_back(graphNode); GeoIntrusivePtr<GeoGraphNode> nodePtr{graphNode};
m_daughters.emplace_back(nodePtr);
graphNode->dockTo(this); graphNode->dockTo(this);
} }
......
...@@ -13,15 +13,13 @@ ...@@ -13,15 +13,13 @@
GeoPhysVol::GeoPhysVol(const GeoLogVol* LogVol) GeoPhysVol::GeoPhysVol(const GeoLogVol* LogVol)
: GeoVPhysVol(LogVol) : GeoVPhysVol(LogVol) {}
{
}
void GeoPhysVol::add(GeoGraphNode* graphNode) void GeoPhysVol::add(GeoGraphNode* graphNode) {
{
std::scoped_lock<std::mutex> lk(m_muxVec); std::scoped_lock<std::mutex> lk(m_muxVec);
m_daughters.push_back(graphNode); GeoIntrusivePtr<GeoGraphNode> nodePtr{graphNode};
m_daughters.emplace_back(nodePtr);
graphNode->dockTo(this); graphNode->dockTo(this);
} }
......
// Copyright (C) 2002-2023 CERN for the benefit of the ATLAS collaboration
#include <GeoModelKernel/GeoFullPhysVol.h>
#include <GeoModelKernel/GeoPhysVol.h>
#include <GeoModelKernel/GeoBox.h>
#include <iostream>
#define PRINT(OBJ) \
std::cout<<__FILE__<<":"<<__LINE__<<" Print reference count of "<<(RCBase*)OBJ; \
if (OBJ) std::cout<<" "<<typeid(*OBJ).name()<<" "<<OBJ->refCount(); \
std::cout<<std::endl; \
#define CHECKCOUNT(OBJ, EXPECT) \
if (OBJ->refCount() != EXPECT) { \
PRINT(OBJ); \
std::cerr<<__FILE__<<":"<<__LINE__<<" Expect a reference count of "<<EXPECT<<std::endl; \
return EXIT_FAILURE; \
} \
PRINT(OBJ); \
std::cout<<__FILE__<<":"<<__LINE__<<" Pass ref counter check of "<<EXPECT<<std::endl; \
int main(int argc, char *argv[]){
GeoMaterial* material = new GeoMaterial("Snow", 1.45);
GeoBox* uniCave = new GeoBox(100., 100., 100.);
/// Create the logical volume forklift which should increase the counters by one unit
GeoLogVol* logVol = new GeoLogVol("Forklift", uniCave, material);
CHECKCOUNT(material, 1);
CHECKCOUNT(uniCave, 1);
/// The logical volume should be also incremented by one unit then
GeoIntrusivePtr<GeoFullPhysVol> myPhysVol{new GeoFullPhysVol(logVol)};
CHECKCOUNT(myPhysVol, 1);
CHECKCOUNT(logVol, 1);
/// Temporarilly create a new logical volume pointer
{
GeoIntrusivePtr<GeoLogVol> logInt{logVol};
CHECKCOUNT(logInt, 2);
}
/// Another check that the logical volume is back to 1
CHECKCOUNT(logVol, 1);
{
/// Derived constructor from the mother
GeoIntrusivePtr<GeoVFullPhysVol> physVol2{myPhysVol};
CHECKCOUNT(physVol2, 2);
/// Test move constructor
GeoIntrusivePtr<GeoVPhysVol> physVol3{std::move(physVol2)};
CHECKCOUNT(physVol3, 2);
/// Test the copy assignment
GeoIntrusivePtr<GeoVPhysVol> physVol4{};
physVol4 = physVol3;
CHECKCOUNT(physVol4, 3);
/// Reset
physVol4.reset();
// We should be back to 2
CHECKCOUNT(physVol3, 2);
/// Move assignment
physVol4 = std::move(physVol3);
CHECKCOUNT(physVol4, 2);
}
return EXIT_SUCCESS;
}
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment