[libc++] Fix __hash_table::erase(iterator, iterator) to update the bucket list correctly when erasing the last bucket

[philnik777]

Fixes #167820

[rupprecht]

Noticed this was mentioned on the issue I filed and decided to give this a shot. This fixes all the regressions I've found so far. Thanks!

[rupprecht]

Looks like CI doesn't like this for older versions

  # | /home/gha/actions-runner/_work/llvm-project/llvm-project/libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp:56:12: error: decomposition declarations are a C++17 extension [-Werror,-Wc++17-extensions]
  # |    56 |       auto [it, end] = m.equal_range(1);

[alexfh]

Thanks for the fix! LGTM

[asmok-g]

Thanks @philnik777 for the fix! Can we have this merged now ? Thanks.

[philnik777]

Thanks @philnik777 for the fix! Can we have this merged now ? Thanks.

I'll talk with Louis in ~an hour. I'll try to land it after.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Fixes #167820

---

Full diff: https://github.com/llvm/llvm-project/pull/167865.diff

5 Files Affected:

• (modified) libcxx/include/__hash_table (+2)
• (modified) libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/erase_range.pass.cpp (+22)
• (modified) libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp (+22)
• (modified) libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp (+22)
• (modified) libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp (+22)

diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index e1897949a47e6..ef487fb06dd5e 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -1910,6 +1910,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __first, const_it
         __bucket_list_[__next_chash] = __before_first;
         __chash                      = __next_chash;
       }
+    } else { // When __next is a nullptr we've fully erased the last bucket. Update the bucket list accordingly.
+      __bucket_list_[__chash] = nullptr;
     }
   }
 
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/erase_range.pass.cpp
index 532413437f6be..81371638143c9 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/erase_range.pass.cpp
@@ -57,6 +57,28 @@ int main(int, char**) {
     assert(c.size() == 0);
     assert(k == c.end());
   }
+  { // Make sure that we're properly updating the bucket list when we're erasing to the end
+    std::unordered_map<int, int> m;
+    m.insert(std::make_pair(1, 1));
+    m.insert(std::make_pair(2, 2));
+
+    {
+      auto pair = m.equal_range(1);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    {
+      auto pair = m.equal_range(2);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    m.insert(std::make_pair(3, 3));
+    assert(m.size() == 1);
+    assert(*m.begin() == std::make_pair(3, 3));
+    assert(++m.begin() == m.end());
+  }
 #if TEST_STD_VER >= 11
   {
     typedef std::unordered_map<int,
diff --git a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
index 38b75c0c1986b..aa6bc20e4090b 100644
--- a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
@@ -122,6 +122,28 @@ int main(int, char**) {
     for (const auto& v : map)
       assert(v.first == 1 || v.first == collision_val);
   }
+  { // Make sure that we're properly updating the bucket list when we're erasing to the end
+    std::unordered_multimap<int, int> m;
+    m.insert(std::make_pair(1, 1));
+    m.insert(std::make_pair(2, 2));
+
+    {
+      auto pair = m.equal_range(1);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    {
+      auto pair = m.equal_range(2);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    m.insert(std::make_pair(3, 3));
+    assert(m.size() == 1);
+    assert(*m.begin() == std::make_pair(3, 3));
+    assert(++m.begin() == m.end());
+  }
 #if TEST_STD_VER >= 11
   {
     typedef std::unordered_multimap<int,
diff --git a/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
index 3bc686ec2d86e..013e052e530de 100644
--- a/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
@@ -64,6 +64,28 @@ int main(int, char**) {
     for (const auto& v : map)
       assert(v == 1 || v == collision_val);
   }
+  { // Make sure that we're properly updating the bucket list when we're erasing to the end
+    std::unordered_multiset<int> m;
+    m.insert(1);
+    m.insert(2);
+
+    {
+      auto pair = m.equal_range(1);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    {
+      auto pair = m.equal_range(2);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    m.insert(3);
+    assert(m.size() == 1);
+    assert(*m.begin() == 3);
+    assert(++m.begin() == m.end());
+  }
 #if TEST_STD_VER >= 11
   {
     typedef std::unordered_multiset<int, std::hash<int>, std::equal_to<int>, min_allocator<int>> C;
diff --git a/libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp
index 5fa6e4199f756..1f049a295b8c3 100644
--- a/libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.set/erase_range.pass.cpp
@@ -47,6 +47,28 @@ int main(int, char**) {
     assert(c.size() == 0);
     assert(k == c.end());
   }
+  { // Make sure that we're properly updating the bucket list when we're erasing to the end
+    std::unordered_set<int> m;
+    m.insert(1);
+    m.insert(2);
+
+    {
+      auto pair = m.equal_range(1);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    {
+      auto pair = m.equal_range(2);
+      assert(pair.first != pair.second);
+      m.erase(pair.first, pair.second);
+    }
+
+    m.insert(3);
+    assert(m.size() == 1);
+    assert(*m.begin() == 3);
+    assert(++m.begin() == m.end());
+  }
 #if TEST_STD_VER >= 11
   {
     typedef std::unordered_set<int, std::hash<int>, std::equal_to<int>, min_allocator<int>> C;

[asmok-g]

Thanks for the merge!!