Skip to content

Commit

Permalink
refactor!: Use consteval on hashString (#3833)
Browse files Browse the repository at this point in the history
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.

BREAKING CHANGES:

The `hashString` function becomes `consteval`, so requires compile-time execution. Use this if at all possible! If not, switch to `hashStringDynamic`, which remains `constexpr` and can thus be executed at runtime.
  • Loading branch information
paulgessinger authored Nov 18, 2024
1 parent fdb79a2 commit 387d0ad
Show file tree
Hide file tree
Showing 12 changed files with 23 additions and 17 deletions.
2 changes: 1 addition & 1 deletion Core/include/Acts/EventData/ProxyAccessor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Core/include/Acts/EventData/TrackContainer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/EventData/TrackProxy.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ class TrackProxy {
constexpr T& component(std::string_view key)
requires(!ReadOnly)
{
return m_container->template component<T>(hashString(key), m_index);
return m_container->template component<T>(hashStringDynamic(key), m_index);
}

/// Retrieve a const reference to a component
Expand Down Expand Up @@ -738,7 +738,7 @@ class TrackProxy {
/// @return Const reference to the component given by @p key
template <typename T>
constexpr const T& component(std::string_view key) const {
return m_container->template component<T>(hashString(key), m_index);
return m_container->template component<T>(hashStringDynamic(key), m_index);
}

/// @}
Expand Down
6 changes: 3 additions & 3 deletions Core/include/Acts/EventData/TrackStateProxy.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1135,7 +1135,7 @@ class TrackStateProxy {
constexpr T& component(std::string_view key)
requires(!ReadOnly)
{
return m_traj->template component<T>(hashString(key), m_istate);
return m_traj->template component<T>(hashStringDynamic(key), m_istate);
}

/// Retrieve a const reference to a component
Expand Down Expand Up @@ -1163,7 +1163,7 @@ class TrackStateProxy {
/// @return Const reference to the component given by @p key
template <typename T>
constexpr const T& component(std::string_view key) const {
return m_traj->template component<T>(hashString(key), m_istate);
return m_traj->template component<T>(hashStringDynamic(key), m_istate);
}

/// @}
Expand Down
2 changes: 1 addition & 1 deletion Core/include/Acts/EventData/VectorMultiTrajectory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ class VectorMultiTrajectory final

template <typename T>
void addColumn_impl(std::string_view key) {
HashedString hashedKey = hashString(key);
HashedString hashedKey = hashStringDynamic(key);
m_dynamic.insert({hashedKey, std::make_unique<detail::DynamicColumn<T>>()});
}

Expand Down
2 changes: 1 addition & 1 deletion Core/include/Acts/EventData/VectorTrackContainer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class VectorTrackContainer final : public detail_vtc::VectorTrackContainerBase {

template <typename T>
constexpr void addColumn_impl(const std::string_view& key) {
HashedString hashedKey = hashString(key);
HashedString hashedKey = hashStringDynamic(key);
m_dynamic.insert({hashedKey, std::make_unique<detail::DynamicColumn<T>>()});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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; });
Expand Down
8 changes: 7 additions & 1 deletion Core/include/Acts/Utilities/HashedString.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@

#pragma once

#include <csignal>
#include <cstddef>
#include <cstdint>
#include <string_view>
#include <type_traits>
#include <utility>

namespace Acts {
Expand All @@ -35,7 +37,11 @@ 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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ class MutablePodioTrackContainer : public PodioTrackContainerBase {

template <typename T>
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<podio_detail::DynamicColumn<T>>(key)});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ class MutablePodioTrackStateContainer final

template <typename T>
constexpr void addColumn_impl(std::string_view key) {
HashedString hashedKey = hashString(key);
HashedString hashedKey = hashStringDynamic(key);
m_dynamic.insert(
{hashedKey, std::make_unique<podio_detail::DynamicColumn<T>>(key)});
}
Expand Down
2 changes: 1 addition & 1 deletion Plugins/Podio/src/PodioUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)});
}
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/UnitTests/Core/Utilities/HashedStringTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down

0 comments on commit 387d0ad

Please sign in to comment.