diff --git a/absl/container/BUILD.bazel b/absl/container/BUILD.bazel index febb9a7f2de..009e67c21ef 100644 --- a/absl/container/BUILD.bazel +++ b/absl/container/BUILD.bazel @@ -305,6 +305,7 @@ cc_test( deps = [ ":container_memory", ":flat_hash_set", + ":hash_container_defaults", ":hash_generator_testing", ":test_allocator", ":unordered_set_constructor_test", @@ -312,6 +313,7 @@ cc_test( ":unordered_set_members_test", ":unordered_set_modifiers_test", "//absl/base:config", + "//absl/hash", "//absl/log:check", "//absl/memory", "//absl/strings", diff --git a/absl/container/CMakeLists.txt b/absl/container/CMakeLists.txt index 68a12eb2242..b9152d24f67 100644 --- a/absl/container/CMakeLists.txt +++ b/absl/container/CMakeLists.txt @@ -350,6 +350,8 @@ absl_cc_test( absl::config absl::container_memory absl::flat_hash_set + absl::hash + absl::hash_container_defaults absl::hash_generator_testing absl::memory absl::strings diff --git a/absl/container/flat_hash_set_test.cc b/absl/container/flat_hash_set_test.cc index 0dd43269edf..96d77440e35 100644 --- a/absl/container/flat_hash_set_test.cc +++ b/absl/container/flat_hash_set_test.cc @@ -14,6 +14,7 @@ #include "absl/container/flat_hash_set.h" +#include #include #include #include @@ -23,6 +24,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/base/config.h" +#include "absl/container/hash_container_defaults.h" #include "absl/container/internal/container_memory.h" #include "absl/container/internal/hash_generator_testing.h" #include "absl/container/internal/test_allocator.h" @@ -30,6 +32,7 @@ #include "absl/container/internal/unordered_set_lookup_test.h" #include "absl/container/internal/unordered_set_members_test.h" #include "absl/container/internal/unordered_set_modifiers_test.h" +#include "absl/hash/hash.h" #include "absl/log/check.h" #include "absl/memory/memory.h" #include "absl/strings/string_view.h" @@ -291,6 +294,49 @@ TEST(FlatHashSet, FlatHashSetPolicyDestroyReturnsTrue) { std::allocator>(nullptr, nullptr))())); } +struct HashEqInvalidOnMove { + HashEqInvalidOnMove() = default; + HashEqInvalidOnMove(const HashEqInvalidOnMove& rhs) = default; + HashEqInvalidOnMove(HashEqInvalidOnMove&& rhs) { rhs.moved = true; } + HashEqInvalidOnMove& operator=(const HashEqInvalidOnMove& rhs) = default; + HashEqInvalidOnMove& operator=(HashEqInvalidOnMove&& rhs) { + rhs.moved = true; + return *this; + } + + size_t operator()(int x) const { + CHECK(!moved); + return absl::HashOf(x); + } + + bool operator()(int x, int y) const { + CHECK(!moved); + return x == y; + } + + bool moved = false; +}; + +TEST(FlatHashSet, MovedFromCleared_HashMustBeValid) { + flat_hash_set s1, s2; + // Moving the hashtable must not move the hasher because we need to support + // this behavior. + s2 = std::move(s1); + s1.clear(); + s1.insert(2); + EXPECT_THAT(s1, UnorderedElementsAre(2)); +} + +TEST(FlatHashSet, MovedFromCleared_EqMustBeValid) { + flat_hash_set, HashEqInvalidOnMove> s1, s2; + // Moving the hashtable must not move the equality functor because we need to + // support this behavior. + s2 = std::move(s1); + s1.clear(); + s1.insert(2); + EXPECT_THAT(s1, UnorderedElementsAre(2)); +} + } // namespace } // namespace container_internal ABSL_NAMESPACE_END diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h index 3cb942238ba..2c44f602cbf 100644 --- a/absl/container/internal/raw_hash_set.h +++ b/absl/container/internal/raw_hash_set.h @@ -2841,7 +2841,6 @@ class raw_hash_set { : // Hash, equality and allocator are copied instead of moved because // `that` must be left valid. If Hash is std::function, moving it // would create a nullptr functor that cannot be called. - // TODO(b/296061262): move instead of copying hash/eq/alloc. // Note: we avoid using exchange for better generated code. settings_(PolicyTraits::transfer_uses_memcpy() || !that.is_full_soo() ? std::move(that.common()) @@ -3831,7 +3830,6 @@ class raw_hash_set { destructor_impl(); move_common(that.is_full_soo(), that.alloc_ref(), common(), std::move(that.common())); - // TODO(b/296061262): move instead of copying hash/eq/alloc. hash_ref() = that.hash_ref(); eq_ref() = that.eq_ref(); CopyAlloc(alloc_ref(), that.alloc_ref(), @@ -3870,7 +3868,6 @@ class raw_hash_set { // We can't take over that's memory so we need to move each element. // While moving elements, this should have that's hash/eq so copy hash/eq // before moving elements. - // TODO(b/296061262): move instead of copying hash/eq. hash_ref() = that.hash_ref(); eq_ref() = that.eq_ref(); return move_elements_allocs_unequal(std::move(that));