Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hash_value template resolution #180

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Fix hash_value template resolution #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d7becd
Fix test case not actually doing any testing
Krzmbrzl Feb 6, 2024
9f87160
Fix broken lookup of hash_value function
Krzmbrzl Feb 8, 2024
074dc6a
assume boost::hash_value takes const T&
evaleev Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 27 additions & 41 deletions SeQuant/core/hash.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ constexpr hash::Impl hash_version() {
#endif
}

class Expr;

namespace detail {
template <typename T, typename Enabler = void>
struct has_hash_value_member_fn_helper : public std::false_type {};
Expand All @@ -54,30 +52,15 @@ template <typename T>
static constexpr bool has_hash_value_member_fn_v =
detail::has_hash_value_member_fn_helper<T>::value;

#ifdef SEQUANT_USE_SYSTEM_BOOST_HASH
#if SEQUANT_BOOST_VERSION < 108100
template <typename T,
typename = std::enable_if_t<has_hash_value_member_fn_v<T>>>
auto hash_value(const T& obj) {
return obj.hash_value();
}
#endif
#endif // SEQUANT_USE_SYSTEM_BOOST_HASH

namespace detail {

template <typename T, typename = std::void_t<>>
struct has_boost_hash_value : std::false_type {};

#ifdef SEQUANT_USE_SYSTEM_BOOST_HASH
template <typename T>
struct has_boost_hash_value<
T, std::void_t<decltype(sequant_boost::hash_value(std::declval<T>()))>>
struct has_boost_hash_value<T, std::void_t<decltype(sequant_boost::hash_value(
std::declval<const T&>()))>>
: std::true_type {};
#endif // SEQUANT_USE_SYSTEM_BOOST_HASH

template <typename T>
constexpr bool has_boost_hash_value_v = has_boost_hash_value<const T&>::value;

template <typename T, typename = std::void_t<>>
struct has_hash_value : std::false_type {};
Expand All @@ -87,12 +70,30 @@ struct has_hash_value<
T, std::void_t<decltype(hash_value(std::declval<const T&>()))>>
: std::true_type {};

} // namespace detail

template <typename T>
constexpr bool has_hash_value_v = has_hash_value<T>::value;
constexpr bool has_boost_hash_value_v =
detail::has_boost_hash_value<const T&>::value;

} // namespace detail
template <typename T>
constexpr bool has_hash_value_v = detail::has_hash_value<T>::value;

// hash_value specialization for types that have a hash_value member function
template <typename T,
std::enable_if_t<has_hash_value_member_fn_v<T>, short> = 0>
auto hash_value(const T& obj) {
return obj.hash_value();
}

using sequant_boost::hash_value;
// hash_value specialization that don't have a hash_value member function but
// have an applicable boost::hash_value function
template <typename T, std::enable_if_t<!has_hash_value_member_fn_v<T> &&
has_boost_hash_value_v<T>,
int> = 0>
auto hash_value(const T& obj) {
return sequant_boost::hash_value(obj);
}

// clang-format off
// rationale:
Expand Down Expand Up @@ -187,13 +188,7 @@ inline void range(std::size_t& seed, It first, It last) {
template <typename It>
std::size_t hash_range(It begin, It end) {
if (begin != end) {
std::size_t seed;
if constexpr (has_hash_value_member_fn_v<std::decay_t<decltype(*begin)>>)
seed = begin->hash_value();
else {
using sequant_boost::hash_value;
[[maybe_unused]] std::size_t seed = hash_value(*begin);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a functional change here where previously the hash-value of *begin would be computed, but the result would be discarded and the original seed would be left in an uninitialized state. This seemed like a bug to me, so I changed it but you'll probably want to double-check that this was not intended behavior after all 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, late night coding, ignored what the compiler was trying to tell me ... 5ff476d

}
std::size_t seed = hash_value(*begin);
sequant_boost::hash_range(seed, begin + 1, end);
return seed;
} else
Expand All @@ -207,22 +202,13 @@ void hash_range(size_t& seed, It begin, It end) {
}

template <typename T>
struct _<
T, std::enable_if_t<!(detail::has_hash_value_v<T>)&&meta::is_range_v<T>>> {
struct _<T, std::enable_if_t<!(has_hash_value_v<T>)&&meta::is_range_v<T>>> {
std::size_t operator()(T const& v) const { return range(begin(v), end(v)); }
};

template <typename T>
struct _<T, std::enable_if_t<!(
!(detail::has_hash_value_v<T>)&&meta::is_range_v<T>)>> {
std::size_t operator()(T const& v) const {
if constexpr (has_hash_value_member_fn_v<T>)
return v.hash_value();
else {
using sequant_boost::hash_value;
return hash_value(v);
}
}
struct _<T, std::enable_if_t<!(!(has_hash_value_v<T>)&&meta::is_range_v<T>)>> {
std::size_t operator()(T const& v) const { return hash_value(v); }
};

template <typename T>
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_mbpt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ TEST_CASE("NBodyOp", "[mbpt]") {
if constexpr (hash_version() == hash::Impl::BoostPre181) {
REQUIRE(
to_latex(simplify(f * t * t)) ==
to_latex(ex<Constant>(2) * f * t1 * t2 + f * t1 * t1 + f * t2 * t2));
to_latex(f * t1 * t1 + f * t2 * t2 + ex<Constant>(2) * f * t1 * t2));
} else {
// std::wcout << "to_latex(simplify(f * t * t)): "
// << to_latex(simplify(f * t * t)) << std::endl;
Expand All @@ -290,8 +290,8 @@ TEST_CASE("NBodyOp", "[mbpt]") {

if constexpr (hash_version() == hash::Impl::BoostPre181) {
REQUIRE(to_latex(simplify(f * t * t * t)) ==
to_latex(f * t1 * t1 * t1 + ex<Constant>(3) * f * t1 * t2 * t2 +
f * t2 * t2 * t2 + ex<Constant>(3) * f * t1 * t1 * t2));
to_latex(ex<Constant>(3) * f * t1 * t2 * t2 + f * t2 * t2 * t2 +
ex<Constant>(3) * f * t1 * t1 * t2 + f * t1 * t1 * t1));
} else {
// std::wcout << "to_latex(simplify(f * t * t * t): "
// << to_latex(simplify(f * t * t * t)) << std::endl;
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_tensor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ TEST_CASE("Tensor", "[elements]") {
auto t2 = Tensor(L"F", {L"i_2"}, {L"i_1"});
size_t t2_hash;
REQUIRE_NOTHROW(t2_hash = hash_value(t2));
REQUIRE_NOTHROW(t1_hash != t2_hash);
REQUIRE(t1_hash != t2_hash);

} // SECTION("hash")

Expand Down
Loading