From 87bd49980f8c3ac426d68a2b0d6c9028be96decc Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Fri, 18 Oct 2024 09:35:38 +0200 Subject: [PATCH 1/9] do it --- cmake/ActsCompilerOptions.cmake | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/cmake/ActsCompilerOptions.cmake b/cmake/ActsCompilerOptions.cmake index 64467c64665..016f4bf065e 100644 --- a/cmake/ActsCompilerOptions.cmake +++ b/cmake/ActsCompilerOptions.cmake @@ -13,6 +13,18 @@ set(cxx_flags "-Wall -Wextra -Wpedantic -Wshadow -Wzero-as-null-pointer-constant -Wold-style-cast" ) +# Add assertions to standard libraries for debug builds +if( + (CMAKE_BUILD_TYPE STREQUAL RelWithDebInfo) + OR (CMAKE_BUILD_TYPE STREQUAL Debug) +) + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") + set(cxx_flags "${cxx_final} -D_GLIBCXX_ASSERTIONS") + elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + set(cxx_flags "${cxx_final} -D_LIBCPP_DEBUG") + endif() +endif() + # This adds some useful conversion checks like float-to-bool, float-to-int, etc. # However, at the moment this is only added to clang builds, since GCC's -Wfloat-conversion # is much more aggressive and also triggers on e.g., double-to-float From 1038fcb9754d4f31ca0ec6c243533c60cabf10cb Mon Sep 17 00:00:00 2001 From: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com> Date: Fri, 18 Oct 2024 09:41:35 +0200 Subject: [PATCH 2/9] fix --- cmake/ActsCompilerOptions.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmake/ActsCompilerOptions.cmake b/cmake/ActsCompilerOptions.cmake index 016f4bf065e..89dccea8bc2 100644 --- a/cmake/ActsCompilerOptions.cmake +++ b/cmake/ActsCompilerOptions.cmake @@ -19,9 +19,9 @@ if( OR (CMAKE_BUILD_TYPE STREQUAL Debug) ) if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") - set(cxx_flags "${cxx_final} -D_GLIBCXX_ASSERTIONS") + set(cxx_flags "${cxx_flags} -D_GLIBCXX_ASSERTIONS") elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") - set(cxx_flags "${cxx_final} -D_LIBCPP_DEBUG") + set(cxx_flags "${cxx_flags} -D_LIBCPP_DEBUG") endif() endif() From e1e47dbeb58fccc63c3e97a2a864b379c1b94ff7 Mon Sep 17 00:00:00 2001 From: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com> Date: Fri, 18 Oct 2024 10:01:07 +0200 Subject: [PATCH 3/9] Update ActsCompilerOptions.cmake --- cmake/ActsCompilerOptions.cmake | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/cmake/ActsCompilerOptions.cmake b/cmake/ActsCompilerOptions.cmake index 89dccea8bc2..44c215ca8d5 100644 --- a/cmake/ActsCompilerOptions.cmake +++ b/cmake/ActsCompilerOptions.cmake @@ -13,16 +13,9 @@ set(cxx_flags "-Wall -Wextra -Wpedantic -Wshadow -Wzero-as-null-pointer-constant -Wold-style-cast" ) -# Add assertions to standard libraries for debug builds -if( - (CMAKE_BUILD_TYPE STREQUAL RelWithDebInfo) - OR (CMAKE_BUILD_TYPE STREQUAL Debug) -) - if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") - set(cxx_flags "${cxx_flags} -D_GLIBCXX_ASSERTIONS") - elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") - set(cxx_flags "${cxx_flags} -D_LIBCPP_DEBUG") - endif() +# Add assertions to standard libraries +if(ACTS_FORCE_ASSERTIONS) + set(cxx_flags "${cxx_flags} -D_GLIBCXX_ASSERTIONS -D_LIBCPP_DEBUG") endif() # This adds some useful conversion checks like float-to-bool, float-to-int, etc. From 257171ede46324a341b42be1b05ee5eae0892112 Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Mon, 9 Dec 2024 17:23:46 +0100 Subject: [PATCH 4/9] fix unittests --- Core/src/Visualization/ObjVisualization3D.cpp | 2 +- .../Core/Geometry/CylinderVolumeBuilderTests.cpp | 6 +++--- .../UnitTests/Core/Utilities/AlgebraHelpersTests.cpp | 11 +++-------- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/Core/src/Visualization/ObjVisualization3D.cpp b/Core/src/Visualization/ObjVisualization3D.cpp index 777e883b3d3..6895ad56f03 100644 --- a/Core/src/Visualization/ObjVisualization3D.cpp +++ b/Core/src/Visualization/ObjVisualization3D.cpp @@ -63,7 +63,7 @@ void ObjVisualization3D::faces(const std::vector& vtxs, o.vertices.insert(o.vertices.end(), vtxs.begin(), vtxs.end()); for (const auto& face : faces) { if (face.size() == 2) { - o.lines.push_back({face[0] + vtxoffs, face[2] + vtxoffs}); + o.lines.push_back({face[0] + vtxoffs, face[1] + vtxoffs}); } else { FaceType rawFace; std::ranges::transform( diff --git a/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp b/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp index e7a96099bb0..9ab6f7392dc 100644 --- a/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp +++ b/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp @@ -22,7 +22,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_wraps, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^ @@ -114,7 +114,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_containes, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^ @@ -285,7 +285,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_overlapsInZ, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^ diff --git a/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp b/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp index 87e47d3e3a9..a821682d795 100644 --- a/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp +++ b/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp @@ -82,18 +82,15 @@ BOOST_AUTO_TEST_CASE(SafeInverseFPESmallMatrix) { m(1, 1) = 1; auto mInv = Acts::safeInverse(m); + BOOST_REQUIRE(mInv.has_value()); auto mInvInv = Acts::safeInverse(*mInv); - - BOOST_CHECK(mInv); BOOST_CHECK(!mInvInv); ACTS_VERBOSE("Test: SafeInverseFPESmallMatrix" << "\n" << "m:\n" << m << "\n" << "mInv:\n" - << *mInv << "\n" - << "mInvInv [garbage]:\n" - << *mInvInv); + << *mInv); } BOOST_AUTO_TEST_CASE(SafeInverseFPELargeMatrix) { @@ -107,9 +104,7 @@ BOOST_AUTO_TEST_CASE(SafeInverseFPELargeMatrix) { ACTS_VERBOSE("Test: SafeInverseFPELargeMatrix" << "\n" << "m:\n" - << m << "\n" - << "mInv [garbage]:\n" - << *mInv); + << m); } /// This test should not compile From 11c99bae5fe87e2e7620e2d01428717cb8dbc204 Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Tue, 10 Dec 2024 15:50:52 +0100 Subject: [PATCH 5/9] fix --- .../include/Acts/EventData/detail/GenerateParameters.hpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Core/include/Acts/EventData/detail/GenerateParameters.hpp b/Core/include/Acts/EventData/detail/GenerateParameters.hpp index b06d853faa6..c4de9cf664f 100644 --- a/Core/include/Acts/EventData/detail/GenerateParameters.hpp +++ b/Core/include/Acts/EventData/detail/GenerateParameters.hpp @@ -106,15 +106,16 @@ inline std::pair generateBoundDirection( // since we want to draw the direction uniform on the unit sphere, we must // draw from cos(theta) instead of theta. see e.g. // https://mathworld.wolfram.com/SpherePointPicking.html - double cosThetaMin = std::cos(options.thetaMin); + // Get cosThetaMin from thetaMax and vice versa + double cosThetaMin = std::cos(options.thetaMax); // ensure upper bound is included. see e.g. // https://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution - double cosThetaMax = std::nextafter(std::cos(options.thetaMax), + double cosThetaMax = std::nextafter(std::cos(options.thetaMin), std::numeric_limits::max()); // in case we force uniform eta generation - double etaMin = Acts::AngleHelpers::etaFromTheta(options.thetaMin); - double etaMax = Acts::AngleHelpers::etaFromTheta(options.thetaMax); + double etaMin = Acts::AngleHelpers::etaFromTheta(options.thetaMax); + double etaMax = Acts::AngleHelpers::etaFromTheta(options.thetaMin); UniformReal phiDist(options.phiMin, options.phiMax); UniformReal cosThetaDist(cosThetaMin, cosThetaMax); From 8ffbd64f0858f30587c94be64eaf9c73863dd003 Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Tue, 10 Dec 2024 17:28:23 +0100 Subject: [PATCH 6/9] update --- Plugins/Json/src/DetrayJsonHelper.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index b04d3e62b11..2b58936745b 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -6,6 +6,14 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +#ifdef _GLIBCXX_ASSERTIONS +#undef _GLIBCXX_ASSERTIONS +#endif + +#ifdef _LIBCPP_DEBUG +#undef _LIBCPP_DEBUG +#endif + #include "Acts/Plugins/Json/DetrayJsonHelper.hpp" namespace Acts::DetrayJsonHelper { From 4d38659fc964dcbc23d3433e3bd272d977fd6bfe Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Tue, 10 Dec 2024 23:00:55 +0100 Subject: [PATCH 7/9] update --- Plugins/Json/src/DetrayJsonHelper.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index 2b58936745b..b067034c914 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -6,14 +6,12 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +// For whatever reason, this compilation unit does not compile +// with those assertions. #ifdef _GLIBCXX_ASSERTIONS #undef _GLIBCXX_ASSERTIONS #endif -#ifdef _LIBCPP_DEBUG -#undef _LIBCPP_DEBUG -#endif - #include "Acts/Plugins/Json/DetrayJsonHelper.hpp" namespace Acts::DetrayJsonHelper { From 4b3af22791c3bd07fa6ada9c712cef67ac1b46ee Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Tue, 10 Dec 2024 23:04:48 +0100 Subject: [PATCH 8/9] update --- Plugins/Json/src/DetrayJsonHelper.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index b067034c914..b41662fb8f2 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -7,7 +7,8 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. // For whatever reason, this compilation unit does not compile -// with those assertions. +// with those assertions and GCC 13. For now just disable the +// flags here. #ifdef _GLIBCXX_ASSERTIONS #undef _GLIBCXX_ASSERTIONS #endif From d98dddd8b3df4a00325aa0fdb3834ac331841523 Mon Sep 17 00:00:00 2001 From: Benjamin Huth Date: Wed, 11 Dec 2024 10:47:55 +0100 Subject: [PATCH 9/9] update --- Core/include/Acts/EventData/detail/GenerateParameters.hpp | 6 +++++- Plugins/Json/src/DetrayJsonHelper.cpp | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Core/include/Acts/EventData/detail/GenerateParameters.hpp b/Core/include/Acts/EventData/detail/GenerateParameters.hpp index c4de9cf664f..630878e7d4a 100644 --- a/Core/include/Acts/EventData/detail/GenerateParameters.hpp +++ b/Core/include/Acts/EventData/detail/GenerateParameters.hpp @@ -102,11 +102,15 @@ template inline std::pair generateBoundDirection( generator_t& rng, const GenerateBoundDirectionOptions& options) { using UniformReal = std::uniform_real_distribution; + assert(options.thetaMin >= 0.f); + assert(options.thetaMax <= std::numbers::pi); + assert(options.thetaMin <= options.thetaMax); // since we want to draw the direction uniform on the unit sphere, we must // draw from cos(theta) instead of theta. see e.g. // https://mathworld.wolfram.com/SpherePointPicking.html - // Get cosThetaMin from thetaMax and vice versa + // Get cosThetaMin from thetaMax and vice versa, because cos is + // monothonical decreasing between [0, pi] double cosThetaMin = std::cos(options.thetaMax); // ensure upper bound is included. see e.g. // https://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index b41662fb8f2..d17e6cee1d5 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -8,8 +8,8 @@ // For whatever reason, this compilation unit does not compile // with those assertions and GCC 13. For now just disable the -// flags here. -#ifdef _GLIBCXX_ASSERTIONS +// flags in this case. +#if defined(_GLIBCXX_ASSERTIONS) && __GNUC__ == 13 #undef _GLIBCXX_ASSERTIONS #endif