Skip to content

Fix robin_hash::erase() method#92

Open
zhicme wants to merge 1 commit intoTessil:masterfrom
zhicme:master
Open

Fix robin_hash::erase() method#92
zhicme wants to merge 1 commit intoTessil:masterfrom
zhicme:master

Conversation

@zhicme
Copy link
Copy Markdown

@zhicme zhicme commented Apr 17, 2026

Bug Description

There is a bug in the range-erase implementation:

if (first == last) {
  return mutable_iterator(first);
}

auto first_mutable = mutable_iterator(first);
auto last_mutable = mutable_iterator(last);
for (auto it = first_mutable.m_bucket; it != last_mutable.m_bucket; ++it) {
  if (!it->empty()) {
    it->clear();
    m_nb_elements--;
  }
}

The problem is that erase(first, last) treats the bucket array as linear rather than circular when clearing the erased range.

As a result, when the erased range logically wraps around the end of the bucket array, the implementation does not correctly account for the circular probe sequence before performing the backward-shift cleanup. This can leave the table in an inconsistent state and may cause incorrect behavior afterward.

In particular, this may lead to a situation where the map ends up containing duplicate keys, which violates the map invariant.

#include <tsl/robin_map.h>
#include <iostream>

struct BadHash {
  std::size_t operator()(int) const noexcept {
    return 6;
  }
};

int main() {
  tsl::robin_map<int, int, BadHash> map(8);
  map.max_load_factor(0.95f);

  map.insert({1, 10});
  map.insert({2, 20});
  map.insert({3, 30});
  map.insert({4, 40});

  for (const auto& kv : map) {
    std::cout << kv.first << ":" << kv.second << "\n";
  }

  map.erase(map.find(1), map.end());
  auto res = map.insert({3, 300});

  std::cout << "inserted_again = " << res.second << "\n";
  std::cout << "size = " << map.size() << "\n";

  for (const auto& kv : map) {
    std::cout << kv.first << ":" << kv.second << "\n";
  }
}
// outputs:
// 3:30
// 4:40
// 1:10
// 2:20
// inserted_again = 1
// size = 3
// 3:30
// 4:40
// 3:300

To fix this issue, I reimplemented erase, added two overloaded version of next_bucket(), and introduced a new helper function cyclic_distance() to correctly handle circular bucket traversal.
I am not sure if there is a better way to fix this issue, but I hope this fix is helpful for you. 😄

@zhicme
Copy link
Copy Markdown
Author

zhicme commented Apr 27, 2026

I’m very sorry, I forgot to switch branches before making the changes. Would you like me to submit a new PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant