feat: harmonize shared ptr usage for surfaces with detector elements (#…

…2451) Prompted through comments in #2439 This PR harmonies the constructor signature for surfaces with detector elements and bounds, it fixes also a bug in the cylinder surface that a `shared_ptr` is first moved then assert checked, if this was the sole occurrence, e.g. by construction `Constructor(std::make_shared<Foo>` the assert would have always fired.
acts-project · Sep 15, 2023 · a9953db · a9953db
1 parent 9e20cea
commit a9953db
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 24 deletions.
diff --git a/Core/include/Acts/Surfaces/CylinderSurface.hpp b/Core/include/Acts/Surfaces/CylinderSurface.hpp
@@ -46,13 +46,6 @@ class CylinderSurface : public Surface {
 #endif
 
  protected:
-  /// Constructor from DetectorElementBase: Element proxy
-  ///
-  /// @param cbounds are the provided cylinder bounds (shared)
-  /// @param detelement is the linked detector element to this surface
-  CylinderSurface(std::shared_ptr<const CylinderBounds> cbounds,
-                  const DetectorElementBase& detelement);
-
   /// Constructor from Transform3 and CylinderBounds
   ///
   /// @param transform The transform to position the surface
@@ -72,7 +65,14 @@ class CylinderSurface : public Surface {
   /// @param cbounds is a shared pointer to a cylindeer bounds object,
   /// it must exist (assert test)
   CylinderSurface(const Transform3& transform,
-                  const std::shared_ptr<const CylinderBounds>& cbounds);
+                  std::shared_ptr<const CylinderBounds> cbounds);
+
+  /// Constructor from DetectorElementBase: Element proxy
+  ///
+  /// @param cbounds are the provided cylinder bounds (shared)
+  /// @param detelement is the linked detector element to this surface
+  CylinderSurface(std::shared_ptr<const CylinderBounds> cbounds,
+                  const DetectorElementBase& detelement);
 
   /// Copy constructor
   ///

diff --git a/Core/include/Acts/Surfaces/DiscSurface.hpp b/Core/include/Acts/Surfaces/DiscSurface.hpp
@@ -91,7 +91,7 @@ class DiscSurface : public Surface {
   ///
   /// @param dbounds The disc bounds describing the surface coverage
   /// @param detelement The detector element represented by this surface
-  DiscSurface(const std::shared_ptr<const DiscBounds>& dbounds,
+  DiscSurface(std::shared_ptr<const DiscBounds> dbounds,
               const DetectorElementBase& detelement);
 
   /// Copy Constructor

diff --git a/Core/include/Acts/Surfaces/PlaneSurface.hpp b/Core/include/Acts/Surfaces/PlaneSurface.hpp
@@ -67,9 +67,9 @@ class PlaneSurface : public Surface {
 
   /// Constructor from DetectorElementBase : Element proxy
   ///
-  /// @param pbounds are the provided planar bounds (shared)
+  /// @param pbounds are the provided planar bounds
   /// @param detelement is the linked detector element to this surface
-  PlaneSurface(const std::shared_ptr<const PlanarBounds>& pbounds,
+  PlaneSurface(std::shared_ptr<const PlanarBounds> pbounds,
                const DetectorElementBase& detelement);
 
   /// Constructor for Planes with (optional) shared bounds object

diff --git a/Core/src/Surfaces/CylinderSurface.cpp b/Core/src/Surfaces/CylinderSurface.cpp
@@ -50,14 +50,13 @@ Acts::CylinderSurface::CylinderSurface(
     const Acts::DetectorElementBase& detelement)
     : Surface(detelement), m_bounds(std::move(cbounds)) {
   /// surfaces representing a detector element must have bounds
-  assert(cbounds);
+  throw_assert(m_bounds, "CylinderBounds must not be nullptr");
 }
 
 Acts::CylinderSurface::CylinderSurface(
-    const Transform3& transform,
-    const std::shared_ptr<const CylinderBounds>& cbounds)
-    : Surface(transform), m_bounds(cbounds) {
-  throw_assert(cbounds, "CylinderBounds must not be nullptr");
+    const Transform3& transform, std::shared_ptr<const CylinderBounds> cbounds)
+    : Surface(transform), m_bounds(std::move(cbounds)) {
+  throw_assert(m_bounds, "CylinderBounds must not be nullptr");
 }
 
 Acts::CylinderSurface& Acts::CylinderSurface::operator=(

diff --git a/Core/src/Surfaces/DiscSurface.cpp b/Core/src/Surfaces/DiscSurface.cpp
@@ -58,10 +58,10 @@ Acts::DiscSurface::DiscSurface(const Transform3& transform,
                                std::shared_ptr<const DiscBounds> dbounds)
     : GeometryObject(), Surface(transform), m_bounds(std::move(dbounds)) {}
 
-Acts::DiscSurface::DiscSurface(const std::shared_ptr<const DiscBounds>& dbounds,
+Acts::DiscSurface::DiscSurface(std::shared_ptr<const DiscBounds> dbounds,
                                const DetectorElementBase& detelement)
-    : GeometryObject(), Surface(detelement), m_bounds(dbounds) {
-  throw_assert(dbounds, "nullptr as DiscBounds");
+    : GeometryObject(), Surface(detelement), m_bounds(std::move(dbounds)) {
+  throw_assert(m_bounds, "nullptr as DiscBounds");
 }
 
 Acts::DiscSurface& Acts::DiscSurface::operator=(const DiscSurface& other) {

diff --git a/Core/src/Surfaces/PlaneSurface.cpp b/Core/src/Surfaces/PlaneSurface.cpp
@@ -56,12 +56,11 @@ Acts::PlaneSurface::PlaneSurface(const Vector3& center, const Vector3& normal)
   m_transform.pretranslate(center);
 }
 
-Acts::PlaneSurface::PlaneSurface(
-    const std::shared_ptr<const PlanarBounds>& pbounds,
-    const Acts::DetectorElementBase& detelement)
-    : Surface(detelement), m_bounds(pbounds) {
+Acts::PlaneSurface::PlaneSurface(std::shared_ptr<const PlanarBounds> pbounds,
+                                 const Acts::DetectorElementBase& detelement)
+    : Surface(detelement), m_bounds(std::move(pbounds)) {
   /// surfaces representing a detector element must have bounds
-  throw_assert(pbounds, "PlaneBounds must not be nullptr");
+  throw_assert(m_bounds, "PlaneBounds must not be nullptr");
 }
 
 Acts::PlaneSurface::PlaneSurface(const Transform3& transform,