From cde37c20a82390ca94f839a0ffe214cecf0f6484 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 8 Nov 2024 16:45:56 +0100 Subject: [PATCH 1/3] refactor: Use `consteval` on `hashString` This allows us to enforce in more places that the hash is calculated at compile time. To make the distinction explicit, I'm adding another function whose name indicates that the hash will be computed at runtime. --- Core/include/Acts/EventData/TrackContainer.hpp | 2 +- Core/include/Acts/EventData/TrackProxy.hpp | 4 ++-- Core/include/Acts/EventData/TrackStateProxy.hpp | 6 +++--- Core/include/Acts/EventData/VectorMultiTrajectory.hpp | 2 +- Core/include/Acts/EventData/VectorTrackContainer.hpp | 2 +- Core/include/Acts/Utilities/HashedString.hpp | 10 ++++++++-- 6 files changed, 16 insertions(+), 10 deletions(-) diff --git a/Core/include/Acts/EventData/TrackContainer.hpp b/Core/include/Acts/EventData/TrackContainer.hpp index f70c430f002..03f7f1f8bb1 100644 --- a/Core/include/Acts/EventData/TrackContainer.hpp +++ b/Core/include/Acts/EventData/TrackContainer.hpp @@ -252,7 +252,7 @@ class TrackContainer { /// Check if this track container has a specific dynamic column /// @param key the key to check for constexpr bool hasColumn(const std::string& key) const { - return m_container->hasColumn_impl(hashString(key)); + return m_container->hasColumn_impl(hashStringDynamic(key)); } /// Check if a this track container has a specific dynamic column diff --git a/Core/include/Acts/EventData/TrackProxy.hpp b/Core/include/Acts/EventData/TrackProxy.hpp index 5939022e2e8..6ccb0f402b2 100644 --- a/Core/include/Acts/EventData/TrackProxy.hpp +++ b/Core/include/Acts/EventData/TrackProxy.hpp @@ -710,7 +710,7 @@ class TrackProxy { constexpr T& component(std::string_view key) requires(!ReadOnly) { - return m_container->template component(hashString(key), m_index); + return m_container->template component(hashStringDynamic(key), m_index); } /// Retrieve a const reference to a component @@ -738,7 +738,7 @@ class TrackProxy { /// @return Const reference to the component given by @p key template constexpr const T& component(std::string_view key) const { - return m_container->template component(hashString(key), m_index); + return m_container->template component(hashStringDynamic(key), m_index); } /// @} diff --git a/Core/include/Acts/EventData/TrackStateProxy.hpp b/Core/include/Acts/EventData/TrackStateProxy.hpp index 34de86cd3d7..741bfa5b511 100644 --- a/Core/include/Acts/EventData/TrackStateProxy.hpp +++ b/Core/include/Acts/EventData/TrackStateProxy.hpp @@ -1101,7 +1101,7 @@ class TrackStateProxy { /// @note This might hash the @p key at runtime instead of compile-time /// @return true if the component exists, false if not constexpr bool has(std::string_view key) const { - return has(hashString(key)); + return has(hashStringDynamic(key)); } /// Retrieve a mutable reference to a component @@ -1135,7 +1135,7 @@ class TrackStateProxy { constexpr T& component(std::string_view key) requires(!ReadOnly) { - return m_traj->template component(hashString(key), m_istate); + return m_traj->template component(hashStringDynamic(key), m_istate); } /// Retrieve a const reference to a component @@ -1163,7 +1163,7 @@ class TrackStateProxy { /// @return Const reference to the component given by @p key template constexpr const T& component(std::string_view key) const { - return m_traj->template component(hashString(key), m_istate); + return m_traj->template component(hashStringDynamic(key), m_istate); } /// @} diff --git a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp index 7d2d05cc9b9..3859239ae69 100644 --- a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp +++ b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp @@ -456,7 +456,7 @@ class VectorMultiTrajectory final template void addColumn_impl(std::string_view key) { - HashedString hashedKey = hashString(key); + HashedString hashedKey = hashStringDynamic(key); m_dynamic.insert({hashedKey, std::make_unique>()}); } diff --git a/Core/include/Acts/EventData/VectorTrackContainer.hpp b/Core/include/Acts/EventData/VectorTrackContainer.hpp index 09ecf7b5b2e..8734f69eb4c 100644 --- a/Core/include/Acts/EventData/VectorTrackContainer.hpp +++ b/Core/include/Acts/EventData/VectorTrackContainer.hpp @@ -225,7 +225,7 @@ class VectorTrackContainer final : public detail_vtc::VectorTrackContainerBase { template constexpr void addColumn_impl(const std::string_view& key) { - HashedString hashedKey = hashString(key); + HashedString hashedKey = hashStringDynamic(key); m_dynamic.insert({hashedKey, std::make_unique>()}); } diff --git a/Core/include/Acts/Utilities/HashedString.hpp b/Core/include/Acts/Utilities/HashedString.hpp index 6cdbe9d21e6..ff6e491d400 100644 --- a/Core/include/Acts/Utilities/HashedString.hpp +++ b/Core/include/Acts/Utilities/HashedString.hpp @@ -8,9 +8,11 @@ #pragma once +#include #include #include #include +#include #include namespace Acts { @@ -35,12 +37,16 @@ constexpr int length(const char* str) { } } // namespace detail -constexpr HashedString hashString(std::string_view s) { +consteval HashedString hashString(std::string_view s) { + return detail::fnv1a_32(s); +} + +constexpr HashedString hashStringDynamic(std::string_view s) { return detail::fnv1a_32(s); } namespace HashedStringLiteral { -constexpr HashedString operator"" _hash(char const* s, std::size_t count) { +consteval HashedString operator"" _hash(char const* s, std::size_t count) { return detail::fnv1a_32(s, count); } From 8c8ac69b1743fe44e5152c3553306baf1dce5b56 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 11 Nov 2024 12:51:05 +0100 Subject: [PATCH 2/3] resolve remaining compile issues --- Core/include/Acts/EventData/ProxyAccessor.hpp | 2 +- .../Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp | 6 +++--- .../include/Acts/Plugins/Podio/PodioTrackContainer.hpp | 2 +- .../include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp | 2 +- Plugins/Podio/src/PodioUtil.cpp | 2 +- Tests/UnitTests/Core/Utilities/HashedStringTests.cpp | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Core/include/Acts/EventData/ProxyAccessor.hpp b/Core/include/Acts/EventData/ProxyAccessor.hpp index 429c0ef670b..d6a9a3497ce 100644 --- a/Core/include/Acts/EventData/ProxyAccessor.hpp +++ b/Core/include/Acts/EventData/ProxyAccessor.hpp @@ -75,7 +75,7 @@ struct ProxyAccessorBase { /// Create the accessor from a string key /// @param _key the key constexpr ProxyAccessorBase(const std::string& _key) - : key{hashString(_key)} {} + : key{hashStringDynamic(_key)} {} /// Access the stored key on the proxy given as an argument. Mutable version /// @tparam proxy_t the type of the proxy diff --git a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp index 93ef7a3eb7c..a2e374c0cc2 100644 --- a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp +++ b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp @@ -1103,8 +1103,8 @@ class MultiTrajectoryTestsCommon { auto test = [&](const std::string& col, auto value) { using T = decltype(value); std::string col2 = col + "_2"; - HashedString h{hashString(col)}; - HashedString h2{hashString(col2)}; + HashedString h{hashStringDynamic(col)}; + HashedString h2{hashStringDynamic(col2)}; trajectory_t traj = m_factory.create(); BOOST_CHECK(!traj.hasColumn(h)); @@ -1188,7 +1188,7 @@ class MultiTrajectoryTestsCommon { } }; - runTest([](const std::string& c) { return hashString(c.c_str()); }); + runTest([](const std::string& c) { return hashStringDynamic(c.c_str()); }); // runTest([](const std::string& c) { return c.c_str(); }); // runTest([](const std::string& c) { return c; }); // runTest([](std::string_view c) { return c; }); diff --git a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp index b6cba524a8e..79ccf2249a1 100644 --- a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp +++ b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackContainer.hpp @@ -238,7 +238,7 @@ class MutablePodioTrackContainer : public PodioTrackContainerBase { template constexpr void addColumn_impl(std::string_view key) { - Acts::HashedString hashedKey = hashString(key); + Acts::HashedString hashedKey = hashStringDynamic(key); m_dynamic.insert( {hashedKey, std::make_unique>(key)}); } diff --git a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp index 8322249a654..e503f3ca2f3 100644 --- a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp +++ b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp @@ -610,7 +610,7 @@ class MutablePodioTrackStateContainer final template constexpr void addColumn_impl(std::string_view key) { - HashedString hashedKey = hashString(key); + HashedString hashedKey = hashStringDynamic(key); m_dynamic.insert( {hashedKey, std::make_unique>(key)}); } diff --git a/Plugins/Podio/src/PodioUtil.cpp b/Plugins/Podio/src/PodioUtil.cpp index 6518fcdff07..7b2fc0668a3 100644 --- a/Plugins/Podio/src/PodioUtil.cpp +++ b/Plugins/Podio/src/PodioUtil.cpp @@ -261,7 +261,7 @@ void recoverDynamicColumns( "' is not of allowed type"}; } - HashedString hashedKey = hashString(dynName); + HashedString hashedKey = hashStringDynamic(dynName); dynamic.insert({hashedKey, std::move(up)}); } } diff --git a/Tests/UnitTests/Core/Utilities/HashedStringTests.cpp b/Tests/UnitTests/Core/Utilities/HashedStringTests.cpp index f38a71cbe89..0045ec4a6dc 100644 --- a/Tests/UnitTests/Core/Utilities/HashedStringTests.cpp +++ b/Tests/UnitTests/Core/Utilities/HashedStringTests.cpp @@ -28,7 +28,7 @@ BOOST_AUTO_TEST_CASE(string_hashes) { BOOST_CHECK_EQUAL("abc"_hash, 440920331); std::string s = "abc"; - BOOST_CHECK_EQUAL(hashString(s), 440920331); + BOOST_CHECK_EQUAL(hashStringDynamic(s), 440920331); constexpr std::string_view sv{"abc"}; BOOST_CHECK_EQUAL(hashString(sv), 440920331); static_assert(hashString(sv) == 440920331, "Invalid"); From 54fef6a528e0ece13db20cc7fe9cd2b96929ddeb Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 11 Nov 2024 17:38:07 +0100 Subject: [PATCH 3/3] move literal back to constexpr --- Core/include/Acts/Utilities/HashedString.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/include/Acts/Utilities/HashedString.hpp b/Core/include/Acts/Utilities/HashedString.hpp index ff6e491d400..88b50a34d05 100644 --- a/Core/include/Acts/Utilities/HashedString.hpp +++ b/Core/include/Acts/Utilities/HashedString.hpp @@ -46,7 +46,7 @@ constexpr HashedString hashStringDynamic(std::string_view s) { } namespace HashedStringLiteral { -consteval HashedString operator"" _hash(char const* s, std::size_t count) { +constexpr HashedString operator"" _hash(char const* s, std::size_t count) { return detail::fnv1a_32(s, count); }