From 83bb7d8cbb2e648b09cc4c79f414e5a861e52b7a Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 16:43:31 +0100 Subject: [PATCH 01/12] particle hypothesis --- Core/include/Acts/EventData/ParticleHypothesis.hpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Core/include/Acts/EventData/ParticleHypothesis.hpp b/Core/include/Acts/EventData/ParticleHypothesis.hpp index f236441ede6..7e0b522b1b8 100644 --- a/Core/include/Acts/EventData/ParticleHypothesis.hpp +++ b/Core/include/Acts/EventData/ParticleHypothesis.hpp @@ -27,7 +27,8 @@ class SinglyChargedParticleHypothesis public: constexpr SinglyChargedParticleHypothesis(PdgParticle absPdg, float mass) : GenericParticleHypothesis(absPdg, mass, {}) {} - SinglyChargedParticleHypothesis(PdgParticle absPdg) + + explicit SinglyChargedParticleHypothesis(PdgParticle absPdg) : GenericParticleHypothesis(absPdg) {} template @@ -70,7 +71,7 @@ class NeutralParticleHypothesis : public GenericParticleHypothesis { public: constexpr NeutralParticleHypothesis(PdgParticle absPdg, float mass) : GenericParticleHypothesis(absPdg, mass, {}) {} - NeutralParticleHypothesis(PdgParticle absPdg) + explicit NeutralParticleHypothesis(PdgParticle absPdg) : GenericParticleHypothesis(absPdg) {} template @@ -102,7 +103,7 @@ class NonNeutralChargedParticleHypothesis constexpr NonNeutralChargedParticleHypothesis(PdgParticle absPdg, float mass, NonNeutralCharge chargeType) : GenericParticleHypothesis(absPdg, mass, chargeType) {} - NonNeutralChargedParticleHypothesis(PdgParticle absPdg) + explicit NonNeutralChargedParticleHypothesis(PdgParticle absPdg) : GenericParticleHypothesis(absPdg) {} template @@ -148,7 +149,8 @@ class ParticleHypothesis : public GenericParticleHypothesis { constexpr ParticleHypothesis(PdgParticle absPdg, float mass, AnyCharge chargeType) : GenericParticleHypothesis(absPdg, mass, chargeType) {} - ParticleHypothesis(PdgParticle absPdg) : GenericParticleHypothesis(absPdg) {} + explicit ParticleHypothesis(PdgParticle absPdg) + : GenericParticleHypothesis(absPdg) {} template constexpr ParticleHypothesis( From 5cbc3a1265064534786edf59fb43723e0e4eb8df Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 18:01:22 +0100 Subject: [PATCH 02/12] surface --- Core/include/Acts/Surfaces/Surface.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Core/include/Acts/Surfaces/Surface.hpp b/Core/include/Acts/Surfaces/Surface.hpp index 6bd1c126a93..0aef846e8f1 100644 --- a/Core/include/Acts/Surfaces/Surface.hpp +++ b/Core/include/Acts/Surfaces/Surface.hpp @@ -82,7 +82,7 @@ class Surface : public virtual GeometryObject, /// /// @param transform Transform3 positions the surface in 3D global space /// @note also acts as default constructor - Surface(const Transform3& transform = Transform3::Identity()); + explicit Surface(const Transform3& transform = Transform3::Identity()); /// Copy constructor /// @@ -90,12 +90,12 @@ class Surface : public virtual GeometryObject, /// to detector element and layer /// /// @param other Source surface for copy. - Surface(const Surface& other); + explicit Surface(const Surface& other); /// Constructor from DetectorElementBase: Element proxy /// /// @param detelement Detector element which is represented by this surface - Surface(const DetectorElementBase& detelement); + explicit Surface(const DetectorElementBase& detelement); /// Copy constructor with optional shift /// From 293092bafea837176d37cbb7c3bd17b008e6e11b Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 18:01:30 +0100 Subject: [PATCH 03/12] TimedOutputDecorator --- Core/include/Acts/Utilities/Logger.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/include/Acts/Utilities/Logger.hpp b/Core/include/Acts/Utilities/Logger.hpp index b9e499b74f3..8a6bc632494 100644 --- a/Core/include/Acts/Utilities/Logger.hpp +++ b/Core/include/Acts/Utilities/Logger.hpp @@ -446,8 +446,8 @@ class TimedOutputDecorator final : public OutputDecorator { /// /// @param [in] wrappee output print policy object to be wrapped /// @param [in] format format of time stamp (see std::strftime) - TimedOutputDecorator(std::unique_ptr wrappee, - const std::string& format = "%X") + explicit TimedOutputDecorator(std::unique_ptr wrappee, + const std::string& format = "%X") : OutputDecorator(std::move(wrappee)), m_format(format) {} /// @brief flush the debug message to the destination stream From 9ca8a058dd2225605f1909626acbc9b3b9d2c34c Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 18:04:21 +0100 Subject: [PATCH 04/12] track parameters --- .../Acts/EventData/GenericBoundTrackParameters.hpp | 8 +++++++- .../Acts/EventData/MultiComponentTrackParameters.hpp | 3 +++ Core/include/Acts/EventData/TrackParametersConcept.hpp | 7 +++++++ Core/include/Acts/Propagator/Propagator.hpp | 8 ++++++-- Core/include/Acts/Propagator/Propagator.ipp | 10 ++++++++-- 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp index bd2d2063831..9c6d5fb052e 100644 --- a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp @@ -9,6 +9,7 @@ #pragma once #include "Acts/Definitions/Tolerance.hpp" +#include "Acts/EventData/ParticleHypothesis.hpp" #include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" @@ -69,12 +70,17 @@ class GenericBoundTrackParameters { /// Converts a bound track parameter with a different hypothesis. template - GenericBoundTrackParameters( + explicit GenericBoundTrackParameters( const GenericBoundTrackParameters& other) : GenericBoundTrackParameters(other.referenceSurface().getSharedPtr(), other.parameters(), other.covariance(), other.particleHypothesis()) {} + /// Convert this track parameter object to the general type-erased one + GenericBoundTrackParameters toBound() const { + return GenericBoundTrackParameters{*this}; + } + /// Factory to construct from four-position, direction, absolute momentum, and /// charge. /// diff --git a/Core/include/Acts/EventData/MultiComponentTrackParameters.hpp b/Core/include/Acts/EventData/MultiComponentTrackParameters.hpp index 5e079af0df7..bf9dea6fa61 100644 --- a/Core/include/Acts/EventData/MultiComponentTrackParameters.hpp +++ b/Core/include/Acts/EventData/MultiComponentTrackParameters.hpp @@ -121,6 +121,9 @@ class MultiComponentBoundTrackParameters { MultiComponentBoundTrackParameters& operator=( MultiComponentBoundTrackParameters&&) = default; + /// Comply with bound convertible, in this case return a copy + MultiComponentBoundTrackParameters toBound() const { return *this; } + /// Access the parameters const auto& components() const { return m_components; } diff --git a/Core/include/Acts/EventData/TrackParametersConcept.hpp b/Core/include/Acts/EventData/TrackParametersConcept.hpp index 4369fe57141..488193e57f3 100644 --- a/Core/include/Acts/EventData/TrackParametersConcept.hpp +++ b/Core/include/Acts/EventData/TrackParametersConcept.hpp @@ -66,4 +66,11 @@ concept BoundTrackParametersConcept = }; }; +namespace Concepts { +template +concept BoundConvertibleTrackParameters = requires(const Parameters &p) { + { p.toBound() } -> BoundTrackParametersConcept; +}; +} // namespace Concepts + } // namespace Acts diff --git a/Core/include/Acts/Propagator/Propagator.hpp b/Core/include/Acts/Propagator/Propagator.hpp index d3664985fbc..49be0abcbf2 100644 --- a/Core/include/Acts/Propagator/Propagator.hpp +++ b/Core/include/Acts/Propagator/Propagator.hpp @@ -292,7 +292,9 @@ class Propagator final template auto makeState(const parameters_t& start, - const propagator_options_t& options) const; + const propagator_options_t& options) const + requires Concepts::BasicTrackParameters && + Concepts::BoundConvertibleTrackParameters; /// @brief Builds the propagator state object /// @@ -315,7 +317,9 @@ class Propagator final typename target_aborter_t = SurfaceReached, typename path_aborter_t = PathLimitReached> auto makeState(const parameters_t& start, const Surface& target, - const propagator_options_t& options) const; + const propagator_options_t& options) const + requires Concepts::BasicTrackParameters && + Concepts::BoundConvertibleTrackParameters; /// @brief Propagate track parameters /// diff --git a/Core/include/Acts/Propagator/Propagator.ipp b/Core/include/Acts/Propagator/Propagator.ipp index 0790a5d906b..c40f61acd1e 100644 --- a/Core/include/Acts/Propagator/Propagator.ipp +++ b/Core/include/Acts/Propagator/Propagator.ipp @@ -230,7 +230,10 @@ template template auto Acts::Propagator::makeState( - const parameters_t& start, const propagator_options_t& options) const { + const parameters_t& start, const propagator_options_t& options) const + requires Concepts::BasicTrackParameters && + Concepts::BoundConvertibleTrackParameters +{ static_assert(BoundTrackParametersConcept, "Parameters do not fulfill bound parameters concept."); @@ -273,7 +276,10 @@ template auto Acts::Propagator::makeState( const parameters_t& start, const Surface& target, - const propagator_options_t& options) const { + const propagator_options_t& options) const + requires Concepts::BasicTrackParameters && + Concepts::BoundConvertibleTrackParameters +{ static_assert(BoundTrackParametersConcept, "Parameters do not fulfill bound parameters concept."); From d41bc2025fbffaa26502ea776034077ae51bd640 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 18:23:59 +0100 Subject: [PATCH 05/12] charge --- Core/include/Acts/EventData/Charge.hpp | 14 +++++++------- Core/include/Acts/EventData/ParticleHypothesis.hpp | 12 +++++++----- Examples/Python/src/EventData.cpp | 10 +++++++--- Fatras/include/ActsFatras/EventData/Particle.hpp | 4 +++- .../Acts/Plugins/Podio/PodioTrackContainer.hpp | 2 +- .../Propagator/VolumeMaterialInteractionTests.cpp | 2 +- 6 files changed, 26 insertions(+), 18 deletions(-) diff --git a/Core/include/Acts/EventData/Charge.hpp b/Core/include/Acts/EventData/Charge.hpp index 64073538a3f..75d32d8f59f 100644 --- a/Core/include/Acts/EventData/Charge.hpp +++ b/Core/include/Acts/EventData/Charge.hpp @@ -60,7 +60,7 @@ struct Neutral { /// Construct and verify the input charge magnitude (in debug builds). /// /// This constructor is only provided to allow consistent construction. - constexpr Neutral(float absQ) noexcept { + constexpr explicit Neutral(float absQ) noexcept { assert((absQ == 0) && "Input charge must be zero"); (void)absQ; } @@ -102,7 +102,7 @@ struct SinglyCharged { /// Construct and verify the input charge magnitude (in debug builds). /// /// This constructor is only provided to allow consistent construction. - constexpr SinglyCharged(float absQ) noexcept { + constexpr explicit SinglyCharged(float absQ) noexcept { assert((absQ == UnitConstants::e) && "Input charge magnitude must be e"); (void)absQ; } @@ -141,10 +141,10 @@ static_assert(ChargeConcept, class NonNeutralCharge { public: /// Construct with the magnitude of the input charge. - constexpr NonNeutralCharge(float absQ) noexcept : m_absQ{absQ} { + constexpr explicit NonNeutralCharge(float absQ) noexcept : m_absQ{absQ} { assert((0 < absQ) && "Input charge magnitude must be positive"); } - constexpr NonNeutralCharge(SinglyCharged /*unused*/) noexcept + constexpr explicit NonNeutralCharge(SinglyCharged /*unused*/) noexcept : m_absQ{UnitConstants::e} {} constexpr float absQ() const noexcept { return m_absQ; } @@ -182,12 +182,12 @@ static_assert(ChargeConcept, class AnyCharge { public: /// Construct with the magnitude of the input charge. - constexpr AnyCharge(float absQ) noexcept : m_absQ{absQ} { + constexpr explicit AnyCharge(float absQ) noexcept : m_absQ{absQ} { assert((0 <= absQ) && "Input charge magnitude must be zero or positive"); } - constexpr AnyCharge(SinglyCharged /*unused*/) noexcept + constexpr explicit AnyCharge(SinglyCharged /*unused*/) noexcept : m_absQ{UnitConstants::e} {} - constexpr AnyCharge(Neutral /*unused*/) noexcept {} + constexpr explicit AnyCharge(Neutral /*unused*/) noexcept {} constexpr float absQ() const noexcept { return m_absQ; } diff --git a/Core/include/Acts/EventData/ParticleHypothesis.hpp b/Core/include/Acts/EventData/ParticleHypothesis.hpp index 7e0b522b1b8..8bbf830023c 100644 --- a/Core/include/Acts/EventData/ParticleHypothesis.hpp +++ b/Core/include/Acts/EventData/ParticleHypothesis.hpp @@ -128,8 +128,8 @@ class NonNeutralChargedParticleHypothesis } static NonNeutralChargedParticleHypothesis pionLike(float absQ) { - return NonNeutralChargedParticleHypothesis(pion().absolutePdg(), - pion().mass(), absQ); + return NonNeutralChargedParticleHypothesis( + pion().absolutePdg(), pion().mass(), NonNeutralCharge{absQ}); } static NonNeutralChargedParticleHypothesis chargedGeantino() { @@ -137,7 +137,8 @@ class NonNeutralChargedParticleHypothesis return cache; } static NonNeutralChargedParticleHypothesis chargedGeantino(float absQ) { - return NonNeutralChargedParticleHypothesis(PdgParticle::eInvalid, 0, absQ); + return NonNeutralChargedParticleHypothesis(PdgParticle::eInvalid, 0, + NonNeutralCharge{absQ}); } }; @@ -181,7 +182,8 @@ class ParticleHypothesis : public GenericParticleHypothesis { } static ParticleHypothesis pionLike(float absQ) { - return ParticleHypothesis(pion().absolutePdg(), pion().mass(), absQ); + return ParticleHypothesis(pion().absolutePdg(), pion().mass(), + AnyCharge{absQ}); } static ParticleHypothesis geantino() { @@ -192,7 +194,7 @@ class ParticleHypothesis : public GenericParticleHypothesis { return cache; } static ParticleHypothesis chargedGeantino(float absQ) { - return ParticleHypothesis(PdgParticle::eInvalid, 0, absQ); + return ParticleHypothesis(PdgParticle::eInvalid, 0, AnyCharge{absQ}); } }; diff --git a/Examples/Python/src/EventData.cpp b/Examples/Python/src/EventData.cpp index 3f8cfa50342..01643473349 100644 --- a/Examples/Python/src/EventData.cpp +++ b/Examples/Python/src/EventData.cpp @@ -25,12 +25,16 @@ void addEventData(Context& ctx) { auto [m, mex] = ctx.get("main", "examples"); py::class_(m, "ParticleHypothesis") - .def(py::init(), py::arg("pdg"), - py::arg("mass"), py::arg("absCharge")) + .def(py::init([](Acts::PdgParticle absPdg, float mass, float absCharge) { + return Acts::ParticleHypothesis(absPdg, mass, + AnyCharge{absCharge}); + }), + py::arg("pdg"), py::arg("mass"), py::arg("absCharge")) .def(py::init([](std::underlying_type_t absPdg, float mass, float absCharge) { return Acts::ParticleHypothesis( - static_cast(absPdg), mass, absCharge); + static_cast(absPdg), mass, + AnyCharge{absCharge}); }), py::arg("absPdg"), py::arg("mass"), py::arg("absCharge")) .def("__str__", diff --git a/Fatras/include/ActsFatras/EventData/Particle.hpp b/Fatras/include/ActsFatras/EventData/Particle.hpp index fbf9244fcea..50511a2b93c 100644 --- a/Fatras/include/ActsFatras/EventData/Particle.hpp +++ b/Fatras/include/ActsFatras/EventData/Particle.hpp @@ -167,7 +167,9 @@ class Particle { /// Particle hypothesis. Acts::ParticleHypothesis hypothesis() const { - return Acts::ParticleHypothesis(absolutePdg(), mass(), absoluteCharge()); + return Acts::ParticleHypothesis( + absolutePdg(), mass(), + Acts::AnyCharge{static_cast(absoluteCharge())}); } /// Particl qOverP. double qOverP() const { diff --git a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp index 79ccf2249a1..944013c40ea 100644 --- a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp +++ b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp @@ -135,7 +135,7 @@ class PodioTrackContainerBase { auto track = instance.m_collection->at(itrack); const auto& src = track.getParticleHypothesis(); return ParticleHypothesis{static_cast(src.absPdg), src.mass, - src.absQ}; + AnyCharge{src.absQ}}; } static void populateSurfaceBuffer( diff --git a/Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp b/Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp index c97f8e4876d..be42bd9b967 100644 --- a/Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp +++ b/Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp @@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(volume_material_interaction_test) { // Create a propagator state State state; state.stepping.particleHypothesis = - ParticleHypothesis(static_cast(11), 10., 9.); + ParticleHypothesis(static_cast(11), 10., AnyCharge{9.}); state.stepping.pos = Vector3(1., 2., 3.); state.stepping.dir = Vector3(4., 5., 6.); state.stepping.t = 7.; From 3dc58e8b7f9f91a5c9f845aad98002d8adbe3975 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 17 Jan 2025 15:05:38 +0100 Subject: [PATCH 06/12] fixes --- .../Acts/EventData/GenericCurvilinearTrackParameters.hpp | 2 +- Core/include/Acts/Propagator/Propagator.ipp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Core/include/Acts/EventData/GenericCurvilinearTrackParameters.hpp b/Core/include/Acts/EventData/GenericCurvilinearTrackParameters.hpp index 9764ad68cfc..06d5d5f47cb 100644 --- a/Core/include/Acts/EventData/GenericCurvilinearTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericCurvilinearTrackParameters.hpp @@ -70,7 +70,7 @@ class GenericCurvilinearTrackParameters /// Converts a bound track parameter with a different hypothesis. template - GenericCurvilinearTrackParameters( + explicit GenericCurvilinearTrackParameters( const GenericCurvilinearTrackParameters& other) : GenericCurvilinearTrackParameters(other.fourPosition(), diff --git a/Core/include/Acts/Propagator/Propagator.ipp b/Core/include/Acts/Propagator/Propagator.ipp index c40f61acd1e..0795d090ade 100644 --- a/Core/include/Acts/Propagator/Propagator.ipp +++ b/Core/include/Acts/Propagator/Propagator.ipp @@ -258,7 +258,8 @@ auto Acts::Propagator::makeState( actor_list_t_state_t; // Initialize the internal propagator state - StateType state{eOptions, m_stepper.makeState(eOptions.stepping, start), + StateType state{eOptions, + m_stepper.makeState(eOptions.stepping, start.toBound()), m_navigator.makeState(eOptions.navigation)}; static_assert( From 3ab544cec3927e26f2e33d97d523abc178e6e855 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 10 Dec 2024 14:03:04 +0100 Subject: [PATCH 07/12] chore: Add `google-explicit-constructor` to clang-tidy config --- .clang-tidy | 1 + cmake/ActsStaticAnalysis.cmake | 1 + 2 files changed, 2 insertions(+) diff --git a/.clang-tidy b/.clang-tidy index 98e5bf4bb2f..8fab06eb096 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -4,6 +4,7 @@ Checks: '-*, \ cppcoreguidelines-init-variables, \ cppcoreguidelines-pro-type-member-init, \ google-readability-casting, \ + google-explicit-constructor, \ modernize-concat-nested-namespaces, \ modernize-use-equals-default, \ modernize-use-default-member-init, \ diff --git a/cmake/ActsStaticAnalysis.cmake b/cmake/ActsStaticAnalysis.cmake index 1e563a1846b..e16a45e3747 100644 --- a/cmake/ActsStaticAnalysis.cmake +++ b/cmake/ActsStaticAnalysis.cmake @@ -15,6 +15,7 @@ if(ACTS_RUN_CLANG_TIDY) list(APPEND _chks "cppcoreguidelines-init-variables") list(APPEND _chks "cppcoreguidelines-pro-type-member-init") list(APPEND _chks "google-readability-casting") + list(APPEND _chks "google-explicit-constructor") list(APPEND _chks "modernize-concat-nested-namespaces") list(APPEND _chks "modernize-use-equals-default") list(APPEND _chks "modernize-use-default-member-init") From dc07641e878ae4f49e3dc0570fd8cae91f49c33b Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 17 Jan 2025 15:31:51 +0100 Subject: [PATCH 08/12] make GeometryIdentifier constructor explicit --- .../Acts/EventData/detail/TestTrackState.hpp | 4 +- .../Acts/Geometry/GeometryHierarchyMap.hpp | 11 +- .../Acts/Geometry/GeometryIdentifier.hpp | 2 +- .../Acts/Propagator/detail/SteppingLogger.hpp | 2 +- .../src/DigitizationCoordinatesConverter.cpp | 4 +- Examples/Io/Csv/src/CsvMeasurementReader.cpp | 2 +- Examples/Io/Csv/src/CsvMuonSimHitReader.cpp | 3 +- Examples/Io/Root/src/RootSimHitReader.cpp | 2 +- .../Core/Geometry/GeometryIdentifierTests.cpp | 4 +- .../Navigation/DetectorNavigatorTests.cpp | 157 +++++++++++------- .../UnitTests/Core/Seeding/PathSeederTest.cpp | 10 +- 11 files changed, 124 insertions(+), 77 deletions(-) diff --git a/Core/include/Acts/EventData/detail/TestTrackState.hpp b/Core/include/Acts/EventData/detail/TestTrackState.hpp index 26ef78d8084..49ef5c0150d 100644 --- a/Core/include/Acts/EventData/detail/TestTrackState.hpp +++ b/Core/include/Acts/EventData/detail/TestTrackState.hpp @@ -49,8 +49,8 @@ struct TestTrackState { pathLength(std::uniform_real_distribution( 1 * Acts::UnitConstants::mm, 10 * Acts::UnitConstants::mm)(rng)) { // set a random geometry identifier to uniquely identify each surface - auto geoId = - std::uniform_int_distribution()(rng); + Acts::GeometryIdentifier geoId{ + std::uniform_int_distribution()(rng)}; surface->assignGeometryId(geoId); // create source link w/ inline 1d or 2d measurement data diff --git a/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp b/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp index 61cdcdb5e6d..57345198454 100644 --- a/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp +++ b/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp @@ -98,10 +98,12 @@ class GeometryHierarchyMap { /// Access the geometry identifier for the i-th element with bounds check. /// - /// @throws std::out_of_range for invalid indices - GeometryIdentifier idAt(std::size_t index) const { return m_ids.at(index); } + /// @thhrows std::out_of_range for invalid indices + GeometryIdentifier idAt(std::size_t index) const { + return GeometryIdentifier(m_ids.at(index)); + } - /// Access the value of the i-th element in the container with bounds check. + /// Access the value at the given index. /// /// @throws std::out_of_range for invalid indices const Value& valueAt(std::size_t index) const { return m_values.at(index); } @@ -254,7 +256,8 @@ inline void GeometryHierarchyMap::fill( for (const auto& element : elements) { m_ids.push_back(element.first.value()); - m_masks.push_back(makeLeadingLevelsMask(element.first.value())); + m_masks.push_back( + makeLeadingLevelsMask(GeometryIdentifier(element.first.value()))); m_values.push_back(std::move(element.second)); } } diff --git a/Core/include/Acts/Geometry/GeometryIdentifier.hpp b/Core/include/Acts/Geometry/GeometryIdentifier.hpp index 6b43e510f78..0c49febd79f 100644 --- a/Core/include/Acts/Geometry/GeometryIdentifier.hpp +++ b/Core/include/Acts/Geometry/GeometryIdentifier.hpp @@ -32,7 +32,7 @@ class GeometryIdentifier { using Value = std::uint64_t; /// Construct from an already encoded value. - constexpr GeometryIdentifier(Value encoded) : m_value(encoded) {} + explicit constexpr GeometryIdentifier(Value encoded) : m_value(encoded) {} /// Construct default GeometryIdentifier with all values set to zero. GeometryIdentifier() = default; GeometryIdentifier(GeometryIdentifier&&) = default; diff --git a/Core/include/Acts/Propagator/detail/SteppingLogger.hpp b/Core/include/Acts/Propagator/detail/SteppingLogger.hpp index b5b95ff6487..e083d496dfb 100644 --- a/Core/include/Acts/Propagator/detail/SteppingLogger.hpp +++ b/Core/include/Acts/Propagator/detail/SteppingLogger.hpp @@ -37,7 +37,7 @@ struct Step { Vector3 position = Vector3(0., 0., 0.); Vector3 momentum = Vector3(0., 0., 0.); std::shared_ptr surface = nullptr; - GeometryIdentifier geoID = 0; + GeometryIdentifier geoID; /// Note that this is the total number of trials including the previous steps std::size_t nTotalTrials = 0; }; diff --git a/Examples/Algorithms/Digitization/src/DigitizationCoordinatesConverter.cpp b/Examples/Algorithms/Digitization/src/DigitizationCoordinatesConverter.cpp index b996306fb13..06f3dd89d90 100644 --- a/Examples/Algorithms/Digitization/src/DigitizationCoordinatesConverter.cpp +++ b/Examples/Algorithms/Digitization/src/DigitizationCoordinatesConverter.cpp @@ -27,7 +27,7 @@ DigitizationCoordinatesConverter::DigitizationCoordinatesConverter( std::tuple DigitizationCoordinatesConverter::globalToLocal( std::uint64_t moduleId, double x, double y, double z) const { - const Acts::GeometryIdentifier moduleGeoId = moduleId; + const Acts::GeometryIdentifier moduleGeoId(moduleId); auto surfaceItr = m_cfg.surfaceByIdentifier.find(moduleGeoId); if (surfaceItr == m_cfg.surfaceByIdentifier.end()) { throw std::runtime_error("Surface not found for moduleGeoId"); @@ -47,7 +47,7 @@ std::tuple DigitizationCoordinatesConverter::globalToLocal( std::tuple DigitizationCoordinatesConverter::localToGlobal(std::uint64_t moduleId, double x, double y) const { - const Acts::GeometryIdentifier moduleGeoId = moduleId; + const Acts::GeometryIdentifier moduleGeoId{moduleId}; auto surfaceItr = m_cfg.surfaceByIdentifier.find(moduleGeoId); if (surfaceItr == m_cfg.surfaceByIdentifier.end()) { throw std::runtime_error("Surface not found for moduleGeoId"); diff --git a/Examples/Io/Csv/src/CsvMeasurementReader.cpp b/Examples/Io/Csv/src/CsvMeasurementReader.cpp index f426784ea30..6f4e062dc99 100644 --- a/Examples/Io/Csv/src/CsvMeasurementReader.cpp +++ b/Examples/Io/Csv/src/CsvMeasurementReader.cpp @@ -210,7 +210,7 @@ ActsExamples::ProcessCode ActsExamples::CsvMeasurementReader::read( } for (const MeasurementData& m : measurementData) { - Acts::GeometryIdentifier geoId = m.geometry_id; + Acts::GeometryIdentifier geoId{m.geometry_id}; // Create the measurement DigitizedParameters dParameters; diff --git a/Examples/Io/Csv/src/CsvMuonSimHitReader.cpp b/Examples/Io/Csv/src/CsvMuonSimHitReader.cpp index 28836dc48de..c6731decb84 100644 --- a/Examples/Io/Csv/src/CsvMuonSimHitReader.cpp +++ b/Examples/Io/Csv/src/CsvMuonSimHitReader.cpp @@ -79,7 +79,8 @@ ActsExamples::ProcessCode ActsExamples::CsvMuonSimHitReader::read( f.stationPhi = data.StationPhi; f.stationName = data.StationName; - unordered.push_back(SimHit(compressId(f), data.pdgId, pos, mom, mom, -1)); + unordered.push_back(SimHit(Acts::GeometryIdentifier(compressId(f)), + data.pdgId, pos, mom, mom, -1)); } SimHitContainer simHits; simHits.insert(unordered.begin(), unordered.end()); diff --git a/Examples/Io/Root/src/RootSimHitReader.cpp b/Examples/Io/Root/src/RootSimHitReader.cpp index 407c70c5dea..103f4a1140c 100644 --- a/Examples/Io/Root/src/RootSimHitReader.cpp +++ b/Examples/Io/Root/src/RootSimHitReader.cpp @@ -146,7 +146,7 @@ ProcessCode RootSimHitReader::read(const AlgorithmContext& context) { break; } - const Acts::GeometryIdentifier geoid = m_uint64Columns.at("geometry_id"); + const Acts::GeometryIdentifier geoid{m_uint64Columns.at("geometry_id")}; const SimBarcode pid = m_uint64Columns.at("particle_id"); const auto index = m_int32Columns.at("index"); diff --git a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp index b7e7a163d50..5870aa5c1b7 100644 --- a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp +++ b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp @@ -24,7 +24,7 @@ BOOST_AUTO_TEST_CASE(GeometryIdentifier_construct_default) { BOOST_AUTO_TEST_CASE(GeometryIdentifier_construct_encoded) { // not sure if it is a good idea to test for the encoding since it should be // an implementation detail. only the resulting functionality is relevant. - GeometryIdentifier id = 0xa0b00c00d00affe0u; + GeometryIdentifier id{0xa0b00c00d00affe0u}; BOOST_CHECK_EQUAL(id.volume(), 0xa0u); BOOST_CHECK_EQUAL(id.boundary(), 0xb0u); BOOST_CHECK_EQUAL(id.layer(), 0x0c0u); @@ -42,7 +42,7 @@ BOOST_AUTO_TEST_CASE(GeometryIdentifier_max_values) { constexpr GeometryIdentifier::Value sensitiveMax = (1u << 20) - 1; constexpr GeometryIdentifier::Value extraMax = (1u << 8) - 1; // reference values non-zero values everywhere - constexpr GeometryIdentifier ref = 0xdeadaffe01234567; + constexpr GeometryIdentifier ref{0xdeadaffe01234567}; // values above the maximum are truncated // max+1 has all available bits zeroed BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setVolume(volumeMax + 1), diff --git a/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp b/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp index c69bd100ea9..3931dd6c9e5 100644 --- a/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp +++ b/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp @@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsInitialization) { "volume", Acts::Transform3::Identity(), std::move(bounds), {}, {}, Acts::Experimental::tryNoVolumes(), Acts::Experimental::tryAllPortalsAndSurfaces()); - volume->assignGeometryId(1); + volume->assignGeometryId(Acts::GeometryIdentifier(1)); auto detector = Acts::Experimental::Detector::makeShared( "detector", {volume}, Acts::Experimental::tryRootVolumes()); @@ -243,21 +243,21 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsForwardBackward) { // Volume ids: 1-3 for (auto& volume : detectorVolumes) { - volume->assignGeometryId(id); + volume->assignGeometryId(Acts::GeometryIdentifier(id)); id++; } // Intervolume portal ids: 6,7,10,11 for (auto& volume : detectorVolumes) { for (auto& port : volume->portalPtrs()) { - if (port->surface().geometryId() == 0) { - port->surface().assignGeometryId(id); + if (port->surface().geometryId() == Acts::GeometryIdentifier(0)) { + port->surface().assignGeometryId(Acts::GeometryIdentifier(id)); id++; } } } // Surface ids: 12-14 for (auto& surf : {surface1, surface2, surface3}) { - surf->assignGeometryId(id); + surf->assignGeometryId(Acts::GeometryIdentifier(id)); id++; } @@ -316,42 +316,54 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsForwardBackward) { // Action list call before the first step // Starting in the volume1 - BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesFwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesFwd[0].currentPortal, nullptr); // Step to the surface inside volume1 - BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), 12); + BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(12)); BOOST_CHECK_EQUAL(statesFwd[1].currentPortal, nullptr); // Step to the volume1|volume2 boundary (portal has witched volume id) - BOOST_CHECK_EQUAL(statesFwd[2].currentVolume->geometryId(), 2); + BOOST_CHECK_EQUAL(statesFwd[2].currentVolume->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesFwd[2].currentSurface, &(statesFwd[2].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesFwd[2].currentPortal->surface().geometryId(), 7); + BOOST_CHECK_EQUAL(statesFwd[2].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(7)); // Step to the surface inside volume2 - BOOST_CHECK_EQUAL(statesFwd[3].currentVolume->geometryId(), 2); - BOOST_CHECK_EQUAL(statesFwd[3].currentSurface->geometryId(), 13); + BOOST_CHECK_EQUAL(statesFwd[3].currentVolume->geometryId(), + Acts::GeometryIdentifier(2)); + BOOST_CHECK_EQUAL(statesFwd[3].currentSurface->geometryId(), + Acts::GeometryIdentifier(13)); BOOST_CHECK_EQUAL(statesFwd[3].currentPortal, nullptr); // Step to the volume2|volume3 boundary - volume has switched - BOOST_CHECK_EQUAL(statesFwd[4].currentVolume->geometryId(), 3); + BOOST_CHECK_EQUAL(statesFwd[4].currentVolume->geometryId(), + Acts::GeometryIdentifier(3)); BOOST_CHECK_EQUAL(statesFwd[4].currentSurface, &(statesFwd[4].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesFwd[4].currentPortal->surface().geometryId(), 10); + BOOST_CHECK_EQUAL(statesFwd[4].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(10)); // Step to the surface inside volume3 - BOOST_CHECK_EQUAL(statesFwd[5].currentVolume->geometryId(), 3); - BOOST_CHECK_EQUAL(statesFwd[5].currentSurface->geometryId(), 14); + BOOST_CHECK_EQUAL(statesFwd[5].currentVolume->geometryId(), + Acts::GeometryIdentifier(3)); + BOOST_CHECK_EQUAL(statesFwd[5].currentSurface->geometryId(), + Acts::GeometryIdentifier(14)); BOOST_CHECK_EQUAL(statesFwd[5].currentPortal, nullptr); // Step to the volume3|endOfWorld boundary BOOST_CHECK_EQUAL(statesFwd[6].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesFwd[6].currentSurface, &(statesFwd[6].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesFwd[6].currentPortal->surface().geometryId(), 11); + BOOST_CHECK_EQUAL(statesFwd[6].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(11)); // Step to the end of world BOOST_CHECK(navigator.endOfWorldReached(statesFwd[7])); @@ -362,38 +374,50 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsForwardBackward) { BOOST_CHECK_EQUAL(statesBwd[6].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesBwd[6].currentSurface, &(statesBwd[6].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesBwd[6].currentPortal->surface().geometryId(), 6); + BOOST_CHECK_EQUAL(statesBwd[6].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(6)); // Step to the surface inside volume1 - BOOST_CHECK_EQUAL(statesBwd[5].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesBwd[5].currentSurface->geometryId(), 12); + BOOST_CHECK_EQUAL(statesBwd[5].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesBwd[5].currentSurface->geometryId(), + Acts::GeometryIdentifier(12)); BOOST_CHECK_EQUAL(statesBwd[5].currentPortal, nullptr); // Step to the volume1|volume2 boundary / preStep not yet set - BOOST_CHECK_EQUAL(statesBwd[4].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesBwd[4].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesBwd[4].currentSurface, &(statesBwd[4].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesBwd[4].currentPortal->surface().geometryId(), 7); + BOOST_CHECK_EQUAL(statesBwd[4].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(7)); // Step to the surface inside volume2 - BOOST_CHECK_EQUAL(statesBwd[3].currentVolume->geometryId(), 2); - BOOST_CHECK_EQUAL(statesBwd[3].currentSurface->geometryId(), 13); + BOOST_CHECK_EQUAL(statesBwd[3].currentVolume->geometryId(), + Acts::GeometryIdentifier(2)); + BOOST_CHECK_EQUAL(statesBwd[3].currentSurface->geometryId(), + Acts::GeometryIdentifier(13)); BOOST_CHECK_EQUAL(statesBwd[3].currentPortal, nullptr); // Step to the volume2|volume3 boundary / pre-step not yet set - BOOST_CHECK_EQUAL(statesBwd[2].currentVolume->geometryId(), 2); + BOOST_CHECK_EQUAL(statesBwd[2].currentVolume->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesBwd[2].currentSurface, &(statesBwd[2].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesBwd[2].currentPortal->surface().geometryId(), 10); + BOOST_CHECK_EQUAL(statesBwd[2].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(10)); BOOST_CHECK_EQUAL(statesBwd[2].surfaceCandidates.size(), 2u); // Step to the surface inside volume3 - BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), 3); - BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), 14); + BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(3)); + BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(14)); BOOST_CHECK_EQUAL(statesBwd[1].currentPortal, nullptr); // Step to the volume3|endOfWorld boundary - BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), 3); + BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(3)); BOOST_CHECK_EQUAL(statesBwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesBwd[0].currentPortal, nullptr); } @@ -413,11 +437,11 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsAmbiguity) { Acts::Experimental::tryNoVolumes(), Acts::Experimental::tryAllPortalsAndSurfaces()); - volume->assignGeometryId(1); - surface->assignGeometryId(2); + volume->assignGeometryId(Acts::GeometryIdentifier(1)); + surface->assignGeometryId(Acts::GeometryIdentifier(2)); int id = 3; for (auto& port : volume->portalPtrs()) { - port->surface().assignGeometryId(id); + port->surface().assignGeometryId(Acts::GeometryIdentifier(id)); id++; } @@ -473,22 +497,25 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsAmbiguity) { // Action list call before the first step // Starting in the volume - BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesFwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesFwd[0].currentPortal, nullptr); // Step to the cylindrical surface - BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), 2); + BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesFwd[1].currentPortal, nullptr); - BOOST_CHECK_EQUAL(statesFwd[1].surfaceCandidates.size(), 2u); CHECK_CLOSE_REL(statesFwd[1].position.x(), 4, 1e-6); // Step to the volume|endOfWorld boundary BOOST_CHECK_EQUAL(statesFwd[2].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesFwd[2].currentSurface, &(statesFwd[2].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesFwd[2].currentPortal->surface().geometryId(), 6); + BOOST_CHECK_EQUAL(statesFwd[2].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(6)); // Step to the end of world BOOST_CHECK(navigator.endOfWorldReached(statesFwd[3])); @@ -498,17 +525,21 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsAmbiguity) { BOOST_CHECK_EQUAL(statesBwd[2].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesBwd[2].currentSurface, &(statesBwd[2].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesBwd[2].currentPortal->surface().geometryId(), 5); + BOOST_CHECK_EQUAL(statesBwd[2].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(5)); // Step to the cylindrical surface - BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), 2); + BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesBwd[1].currentPortal, nullptr); CHECK_CLOSE_REL(statesBwd[1].position.x(), -4, 1e-6); // Action list call before the first step // Starting in the volume - BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesBwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesBwd[0].currentPortal, nullptr); } @@ -528,11 +559,11 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsMultipleIntersection) { Acts::Experimental::tryNoVolumes(), Acts::Experimental::tryAllPortalsAndSurfaces()); - volume->assignGeometryId(1); - surface->assignGeometryId(2); + volume->assignGeometryId(Acts::GeometryIdentifier(1)); + surface->assignGeometryId(Acts::GeometryIdentifier(2)); int id = 3; for (auto& port : volume->portalPtrs()) { - port->surface().assignGeometryId(id); + port->surface().assignGeometryId(Acts::GeometryIdentifier(id)); id++; } @@ -592,19 +623,24 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsMultipleIntersection) { // Action list call before the first step // Starting in the volume - BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesFwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesFwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesFwd[0].currentPortal, nullptr); // First intersection of the cylindrical surface - BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), 2); + BOOST_CHECK_EQUAL(statesFwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesFwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesFwd[1].currentPortal, nullptr); CHECK_CLOSE_REL(statesFwd[1].position.x(), -4, 1e-6); // Second intersection of the cylindrical surface - BOOST_CHECK_EQUAL(statesFwd[2].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesFwd[2].currentSurface->geometryId(), 2); + BOOST_CHECK_EQUAL(statesFwd[2].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesFwd[2].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesFwd[2].currentPortal, nullptr); CHECK_CLOSE_REL(statesFwd[2].position.x(), 4, 1e-6); @@ -612,7 +648,8 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsMultipleIntersection) { BOOST_CHECK_EQUAL(statesFwd[3].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesFwd[3].currentSurface, &(statesFwd[3].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesFwd[3].currentPortal->surface().geometryId(), 6); + BOOST_CHECK_EQUAL(statesFwd[3].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(6)); // Step to the end of world BOOST_CHECK(navigator.endOfWorldReached(statesFwd[4])); @@ -622,23 +659,29 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsMultipleIntersection) { BOOST_CHECK_EQUAL(statesBwd[3].currentVolume, nullptr); BOOST_CHECK_EQUAL(statesBwd[3].currentSurface, &(statesBwd[3].currentPortal->surface())); - BOOST_CHECK_EQUAL(statesBwd[3].currentPortal->surface().geometryId(), 5); + BOOST_CHECK_EQUAL(statesBwd[3].currentPortal->surface().geometryId(), + Acts::GeometryIdentifier(5)); - // Second intersection of the cylindrical surface - BOOST_CHECK_EQUAL(statesBwd[2].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesBwd[2].currentSurface->geometryId(), 2); + // Step to the cylindrical surface + BOOST_CHECK_EQUAL(statesBwd[2].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesBwd[2].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesBwd[2].currentPortal, nullptr); CHECK_CLOSE_REL(statesBwd[2].position.x(), -4, 1e-6); // First intersection of the cylindrical surface - BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), 1); - BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), 2); + BOOST_CHECK_EQUAL(statesBwd[1].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); + BOOST_CHECK_EQUAL(statesBwd[1].currentSurface->geometryId(), + Acts::GeometryIdentifier(2)); BOOST_CHECK_EQUAL(statesBwd[1].currentPortal, nullptr); CHECK_CLOSE_REL(statesBwd[1].position.x(), 4, 1e-6); // Action list call before the first step // Starting in the volume - BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), 1); + BOOST_CHECK_EQUAL(statesBwd[0].currentVolume->geometryId(), + Acts::GeometryIdentifier(1)); BOOST_CHECK_EQUAL(statesBwd[0].currentSurface, nullptr); BOOST_CHECK_EQUAL(statesBwd[0].currentPortal, nullptr); } diff --git a/Tests/UnitTests/Core/Seeding/PathSeederTest.cpp b/Tests/UnitTests/Core/Seeding/PathSeederTest.cpp index 0162fb78525..34d762a6bdf 100644 --- a/Tests/UnitTests/Core/Seeding/PathSeederTest.cpp +++ b/Tests/UnitTests/Core/Seeding/PathSeederTest.cpp @@ -272,16 +272,16 @@ std::shared_ptr constructTelescopeDetector() { // changes int id = 1; - // Volume ids + // Volume ids: 1-3 for (auto& volume : volumes) { - volume->assignGeometryId(id); + volume->assignGeometryId(Acts::GeometryIdentifier(id)); id++; } - // Intervolume portal ids + // Intervolume portal ids: 6,7,10,11 for (auto& volume : volumes) { for (auto& port : volume->portalPtrs()) { - if (port->surface().geometryId() == 0) { - port->surface().assignGeometryId(id); + if (port->surface().geometryId() == Acts::GeometryIdentifier(0)) { + port->surface().assignGeometryId(Acts::GeometryIdentifier(id)); id++; } } From 243c826900719c8aa214ce6f14caab0b037c09bc Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 17 Jan 2025 16:04:48 +0100 Subject: [PATCH 09/12] fix clang-tidy config format --- .clang-tidy | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 8fab06eb096..8dd2af9f154 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,26 +1,27 @@ --- -Checks: '-*, \ - clang-analyzer-optin.cplusplus.UninitializedObject, \ - cppcoreguidelines-init-variables, \ - cppcoreguidelines-pro-type-member-init, \ - google-readability-casting, \ - google-explicit-constructor, \ - modernize-concat-nested-namespaces, \ - modernize-use-equals-default, \ - modernize-use-default-member-init, \ - modernize-use-nullptr, \ - modernize-use-override, \ - modernize-use-using, \ - performance-for-range-copy, \ - performance-move-const-arg, \ - performance-unnecessary-value-param, \ - readability-braces-around-statements, \ - readability-container-size-empty, \ - readability-implicit-bool-cast, \ - readability-implicit-bool-conversion, \ - readability-inconsistent-declaration-parameter-name, \ - readability-named-parameter, \ - readability-operators-representation' +Checks: > + -*, + clang-analyzer-optin.cplusplus.UninitializedObject, + cppcoreguidelines-init-variables, + cppcoreguidelines-pro-type-member-init, + google-readability-casting, + google-explicit-constructor, + modernize-concat-nested-namespaces, + modernize-use-equals-default, + modernize-use-default-member-init, + modernize-use-nullptr, + modernize-use-override, + modernize-use-using, + performance-for-range-copy, + performance-move-const-arg, + performance-unnecessary-value-param, + readability-braces-around-statements, + readability-container-size-empty, + readability-implicit-bool-cast, + readability-implicit-bool-conversion, + readability-inconsistent-declaration-parameter-name, + readability-named-parameter, + readability-operators-representation HeaderFilterRegex: '.*(? Date: Fri, 17 Jan 2025 16:47:33 +0100 Subject: [PATCH 10/12] progress on explicit --- Core/include/Acts/Geometry/CuboidVolumeBounds.hpp | 2 +- .../Acts/Geometry/CutoutCylinderVolumeBounds.hpp | 4 ++-- Core/include/Acts/Geometry/CylinderVolumeBounds.hpp | 2 +- Core/include/Acts/Geometry/GeometryObject.hpp | 2 +- Core/include/Acts/Surfaces/Surface.hpp | 2 +- Core/include/Acts/Utilities/Axis.hpp | 3 ++- Core/include/Acts/Utilities/BoundingBox.hpp | 2 +- Core/include/Acts/Utilities/Delegate.hpp | 9 ++++++++- Core/include/Acts/Utilities/Grid.hpp | 13 +++++++------ Core/include/Acts/Utilities/RangeXD.hpp | 2 +- Core/include/Acts/Utilities/Result.hpp | 9 ++++++--- .../Acts/Vertexing/GridDensityVertexFinder.hpp | 7 ++++--- Core/src/Geometry/CompositePortalLink.cpp | 7 ++++--- .../Core/Utilities/DelegateChainBuilderTests.cpp | 12 ++++++------ 14 files changed, 45 insertions(+), 31 deletions(-) diff --git a/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp b/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp index 39344352eda..72c0071d131 100644 --- a/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp @@ -69,7 +69,7 @@ class CuboidVolumeBounds : public VolumeBounds { /// Constructor - from a fixed size array /// /// @param values iw the bound values - CuboidVolumeBounds(const std::array& values); + explicit CuboidVolumeBounds(const std::array& values); /// Copy Constructor /// diff --git a/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp b/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp index c90791e376a..a11dc025fdd 100644 --- a/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp @@ -64,8 +64,8 @@ class CutoutCylinderVolumeBounds : public VolumeBounds { /// Constructor - from a fixed size array /// /// @param values The bound values - CutoutCylinderVolumeBounds(const std::array& values) noexcept( - false) + explicit CutoutCylinderVolumeBounds( + const std::array& values) noexcept(false) : m_values(values) { checkConsistency(); buildSurfaceBounds(); diff --git a/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp b/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp index 79916c84083..1477ebf667c 100644 --- a/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp @@ -112,7 +112,7 @@ class CylinderVolumeBounds : public VolumeBounds { /// Constructor - from a fixed size array /// /// @param values The bound values - CylinderVolumeBounds(const std::array& values); + explicit CylinderVolumeBounds(const std::array& values); /// Constructor - extruded from cylinder bounds and thickness /// diff --git a/Core/include/Acts/Geometry/GeometryObject.hpp b/Core/include/Acts/Geometry/GeometryObject.hpp index 191d2234cd5..6c094b17da0 100644 --- a/Core/include/Acts/Geometry/GeometryObject.hpp +++ b/Core/include/Acts/Geometry/GeometryObject.hpp @@ -34,7 +34,7 @@ class GeometryObject { /// Constructor from a value /// /// @param geometryId the geometry identifier of the object - GeometryObject(const GeometryIdentifier& geometryId) + explicit GeometryObject(const GeometryIdentifier& geometryId) : m_geometryId(geometryId) {} /// Assignment operator diff --git a/Core/include/Acts/Surfaces/Surface.hpp b/Core/include/Acts/Surfaces/Surface.hpp index 0aef846e8f1..7cb35afe5b9 100644 --- a/Core/include/Acts/Surfaces/Surface.hpp +++ b/Core/include/Acts/Surfaces/Surface.hpp @@ -90,7 +90,7 @@ class Surface : public virtual GeometryObject, /// to detector element and layer /// /// @param other Source surface for copy. - explicit Surface(const Surface& other); + Surface(const Surface& other); /// Constructor from DetectorElementBase: Element proxy /// diff --git a/Core/include/Acts/Utilities/Axis.hpp b/Core/include/Acts/Utilities/Axis.hpp index c6d5b53a541..ac5428e60e8 100644 --- a/Core/include/Acts/Utilities/Axis.hpp +++ b/Core/include/Acts/Utilities/Axis.hpp @@ -43,7 +43,8 @@ class NeighborHoodIndices { iterator() = default; // Specialized constructor for end() iterator - iterator(std::size_t current) : m_current(current), m_wrapped(true) {} + explicit iterator(std::size_t current) + : m_current(current), m_wrapped(true) {} iterator(std::size_t begin1, std::size_t end1, std::size_t begin2) : m_current(begin1), diff --git a/Core/include/Acts/Utilities/BoundingBox.hpp b/Core/include/Acts/Utilities/BoundingBox.hpp index 3c6f450a1b3..9cebd846c89 100644 --- a/Core/include/Acts/Utilities/BoundingBox.hpp +++ b/Core/include/Acts/Utilities/BoundingBox.hpp @@ -95,7 +95,7 @@ class AxisAlignedBoundingBox { /// contained in @p boxes, and additional envelope can be given. /// @param boxes Vector of child boxes to store in this bounding box. /// @param envelope Envelope that will be added/subtracted to the dimension. - AxisAlignedBoundingBox( + explicit AxisAlignedBoundingBox( const std::vector& boxes, vertex_array_type envelope = vertex_array_type::Zero()); diff --git a/Core/include/Acts/Utilities/Delegate.hpp b/Core/include/Acts/Utilities/Delegate.hpp index 8cac473abb9..af7321b2c96 100644 --- a/Core/include/Acts/Utilities/Delegate.hpp +++ b/Core/include/Acts/Utilities/Delegate.hpp @@ -348,8 +348,15 @@ class OwningDelegate : public Delegate { public: OwningDelegate() = default; - OwningDelegate(Delegate &&delegate) + explicit OwningDelegate( + Delegate &&delegate) : Delegate(std::move(delegate)) {} + + OwningDelegate &operator=( + Delegate &&delegate) { + *this = OwningDelegate{std::move(delegate)}; + return *this; + } }; } // namespace Acts diff --git a/Core/include/Acts/Utilities/Grid.hpp b/Core/include/Acts/Utilities/Grid.hpp index 317fb3b123e..c2bb3b3e9bc 100644 --- a/Core/include/Acts/Utilities/Grid.hpp +++ b/Core/include/Acts/Utilities/Grid.hpp @@ -98,38 +98,39 @@ class Grid final : public IGrid { /// grid object. /// /// @param axes - Grid(const std::tuple& axes) : m_axes(axes) { + explicit Grid(const std::tuple& axes) : m_axes(axes) { m_values.resize(size()); } /// @brief Move constructor from axis tuple /// @param axes - Grid(std::tuple&& axes) : m_axes(std::move(axes)) { + explicit Grid(std::tuple&& axes) : m_axes(std::move(axes)) { m_values.resize(size()); } /// @brief constructor from parameters pack of axes /// @param axes - Grid(Axes&&... axes) : m_axes(std::forward_as_tuple(axes...)) { + explicit Grid(Axes&&... axes) : m_axes(std::forward_as_tuple(axes...)) { m_values.resize(size()); } /// @brief constructor from parameters pack of axes /// @param axes - Grid(const Axes&... axes) : m_axes(std::tuple(axes...)) { + explicit Grid(const Axes&... axes) : m_axes(std::tuple(axes...)) { m_values.resize(size()); } /// @brief constructor from parameters pack of axes and type tag /// @param axes - Grid(TypeTag /*tag*/, Axes&&... axes) + explicit Grid(TypeTag /*tag*/, Axes&&... axes) : m_axes(std::forward_as_tuple(axes...)) { m_values.resize(size()); } /// @brief constructor from parameters pack of axes and type tag /// @param axes - Grid(TypeTag /*tag*/, const Axes&... axes) : m_axes(std::tuple(axes...)) { + explicit Grid(TypeTag /*tag*/, const Axes&... axes) + : m_axes(std::tuple(axes...)) { m_values.resize(size()); } diff --git a/Core/include/Acts/Utilities/RangeXD.hpp b/Core/include/Acts/Utilities/RangeXD.hpp index 19f82245460..3024b230b5f 100644 --- a/Core/include/Acts/Utilities/RangeXD.hpp +++ b/Core/include/Acts/Utilities/RangeXD.hpp @@ -67,7 +67,7 @@ class RangeXD { /// @brief Construct a range from a pair of minimum and maximum values /// @note Only available for one-dimensional ranges /// @param p The pair of minimum and maximum values - RangeXD(const std::pair& p) + explicit RangeXD(const std::pair& p) requires(Dims == 1) : m_minima({p.first}), m_maxima({p.second}) {} diff --git a/Core/include/Acts/Utilities/Result.hpp b/Core/include/Acts/Utilities/Result.hpp index c71836cc788..902e26d110c 100644 --- a/Core/include/Acts/Utilities/Result.hpp +++ b/Core/include/Acts/Utilities/Result.hpp @@ -27,7 +27,7 @@ class Result { /// Private constructor which accepts an external variant. /// This is used by the factory static methods to set up /// the variant unambiguously in all cases. - Result(std::variant&& var) : m_var(std::move(var)) {} + explicit Result(std::variant&& var) : m_var(std::move(var)) {} public: using ValueType = T; @@ -66,7 +66,8 @@ class Result { /// @param value The potential value, could be an actual valid value or an /// error. template - Result(T2 value) noexcept + Result(T2 value) noexcept // NOLINT <- suppress clang-tidy warning. + // Conversion here is crucial for ergonomics requires(!std::same_as && !std::constructible_from && !std::convertible_to && !std::constructible_from && !std::convertible_to && @@ -371,7 +372,9 @@ class Result { /// @tparam E2 The type of the actual error /// @param error The instance of the actual error template - Result(E2 error) noexcept : m_opt(std::move(error)) {} + Result(E2 error) noexcept // NOLINT <- suppress clang-tidy warning. + // Conversion here is crucial for ergonomics + : m_opt(std::move(error)) {} /// Assignment operator from an error. /// @tparam E2 The type of the actual error diff --git a/Core/include/Acts/Vertexing/GridDensityVertexFinder.hpp b/Core/include/Acts/Vertexing/GridDensityVertexFinder.hpp index 9d6c0ee9686..ce6fbca37eb 100644 --- a/Core/include/Acts/Vertexing/GridDensityVertexFinder.hpp +++ b/Core/include/Acts/Vertexing/GridDensityVertexFinder.hpp @@ -37,7 +37,8 @@ class GridDensityVertexFinder final : public IVertexFinder { /// @brief The Config struct struct Config { ///@param gDensity The grid density - Config(GaussianGridTrackDensity gDensity) : gridDensity(gDensity) {} + explicit Config(GaussianGridTrackDensity gDensity) + : gridDensity(gDensity) {} // The grid density object GaussianGridTrackDensity gridDensity; @@ -67,7 +68,7 @@ class GridDensityVertexFinder final : public IVertexFinder { /// /// Only needed if cacheGridStateForTrackRemoval == true struct State { - State(MainGridVector mainGrid_) : mainGrid(std::move(mainGrid_)) {} + explicit State(MainGridVector mainGrid_) : mainGrid(std::move(mainGrid_)) {} // The main density grid MainGridVector mainGrid; @@ -117,7 +118,7 @@ class GridDensityVertexFinder final : public IVertexFinder { /// @brief Constructor for user-defined InputTrack type /// /// @param cfg Configuration object - GridDensityVertexFinder(const Config& cfg) : m_cfg(cfg) { + explicit GridDensityVertexFinder(const Config& cfg) : m_cfg(cfg) { if (!m_cfg.extractParameters.connected()) { throw std::invalid_argument( "GridDensityVertexFinder: " diff --git a/Core/src/Geometry/CompositePortalLink.cpp b/Core/src/Geometry/CompositePortalLink.cpp index 19d9ea3c02a..6032aeaf080 100644 --- a/Core/src/Geometry/CompositePortalLink.cpp +++ b/Core/src/Geometry/CompositePortalLink.cpp @@ -135,9 +135,9 @@ Result CompositePortalLink::resolveVolume( Vector3 global = m_surface->localToGlobal(gctx, position); auto res = resolveVolume(gctx, global, tolerance); if (!res.ok()) { - return res.error(); + return Result::failure(res.error()); } - return *res; + return Result::success(*res); } Result CompositePortalLink::resolveVolume( @@ -153,7 +153,8 @@ Result CompositePortalLink::resolveVolume( } } - return PortalError::PositionNotOnAnyChildPortalLink; + return Result::failure( + PortalError::PositionNotOnAnyChildPortalLink); } std::size_t CompositePortalLink::depth() const { diff --git a/Tests/UnitTests/Core/Utilities/DelegateChainBuilderTests.cpp b/Tests/UnitTests/Core/Utilities/DelegateChainBuilderTests.cpp index 783eb9a85e2..38818d95e81 100644 --- a/Tests/UnitTests/Core/Utilities/DelegateChainBuilderTests.cpp +++ b/Tests/UnitTests/Core/Utilities/DelegateChainBuilderTests.cpp @@ -30,12 +30,12 @@ BOOST_AUTO_TEST_CASE(DelegateChainBuilderAdd) { int x = 0; // Basic building - OwningDelegate chain = DelegateChainBuilder{} - .add<&AddTo::add>(&a1) - .add<&addFive>() - .add<&AddTo::add>(&a2) - .add<&AddTo::add>(&a3) - .build(); + OwningDelegate chain{DelegateChainBuilder{} + .add<&AddTo::add>(&a1) + .add<&addFive>() + .add<&AddTo::add>(&a2) + .add<&AddTo::add>(&a3) + .build()}; chain(x); BOOST_CHECK_EQUAL(x, 11); From 2167c01aba52773f9c6978e653670019997ffc34 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 20 Jan 2025 14:24:27 +0100 Subject: [PATCH 11/12] fix: Blind attempt at fixing neural calib failure --- Examples/Framework/ML/src/NeuralCalibrator.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Examples/Framework/ML/src/NeuralCalibrator.cpp b/Examples/Framework/ML/src/NeuralCalibrator.cpp index 83c7a2bc09a..21fb67cfdb4 100644 --- a/Examples/Framework/ML/src/NeuralCalibrator.cpp +++ b/Examples/Framework/ML/src/NeuralCalibrator.cpp @@ -86,7 +86,8 @@ void ActsExamples::NeuralCalibrator::calibrate( assert((idxSourceLink.index() < measurements.size()) and "Source link index is outside the container bounds"); - if (!rangeContainsValue(m_volumeIds, idxSourceLink.geometryId())) { + if (!Acts::rangeContainsValue(m_volumeIds, + idxSourceLink.geometryId().volume())) { m_fallback.calibrate(measurements, clusters, gctx, cctx, sourceLink, trackState); return; From f1d76682ccf62b7aeb242e1908e6466ce4d2bd6b Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 20 Jan 2025 14:41:43 +0100 Subject: [PATCH 12/12] fix --- Tests/UnitTests/Examples/EventData/MeasurementTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/UnitTests/Examples/EventData/MeasurementTests.cpp b/Tests/UnitTests/Examples/EventData/MeasurementTests.cpp index 1cb5450a792..9769232adf5 100644 --- a/Tests/UnitTests/Examples/EventData/MeasurementTests.cpp +++ b/Tests/UnitTests/Examples/EventData/MeasurementTests.cpp @@ -35,7 +35,7 @@ namespace { constexpr BoundIndices boundIndices[] = { eBoundLoc0, eBoundLoc1, eBoundTime, eBoundPhi, eBoundTheta, eBoundQOverP, }; -constexpr Acts::GeometryIdentifier geoId = 1; +constexpr Acts::GeometryIdentifier geoId{1}; // fix seed for reproducible tests std::default_random_engine rng(123); } // namespace