Improve error messages when dereferencing invalid swisstable iterators.
- Separate the failure cases into different assertions: end/default constructed vs rehashed or erased.
- Update the assertion error for AssertIsValid to not mention the end iterator case because end iterators are considered valid by AssertIsValid.
- Also fix an out-of-date comment for skip_empty_or_deleted.
PiperOrigin-RevId: 485402559
Change-Id: I593056abdc6c3565d0396fb885923fef643bf4e4
diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h
index 93de222..676cebd 100644
--- a/absl/container/internal/raw_hash_set.h
+++ b/absl/container/internal/raw_hash_set.h
@@ -797,15 +797,22 @@
return 0;
}
-#define ABSL_INTERNAL_ASSERT_IS_FULL(ctrl, msg) \
- ABSL_HARDENING_ASSERT((ctrl != nullptr && IsFull(*ctrl)) && msg)
+#define ABSL_INTERNAL_ASSERT_IS_FULL(ctrl, operation) \
+ do { \
+ ABSL_HARDENING_ASSERT( \
+ (ctrl != nullptr) && operation \
+ " called on invalid iterator. The iterator might be an end() " \
+ "iterator or may have been default constructed."); \
+ ABSL_HARDENING_ASSERT( \
+ (IsFull(*ctrl)) && operation \
+ " called on invalid iterator. The element might have been erased or " \
+ "the table might have rehashed."); \
+ } while (0)
inline void AssertIsValid(ctrl_t* ctrl) {
- ABSL_HARDENING_ASSERT(
- (ctrl == nullptr || IsFull(*ctrl)) &&
- "Invalid operation on iterator. The element might have "
- "been erased, the table might have rehashed, or this may "
- "be an end() iterator.");
+ ABSL_HARDENING_ASSERT((ctrl == nullptr || IsFull(*ctrl)) &&
+ "Invalid operation on iterator. The element might have "
+ "been erased or the table might have rehashed.");
}
struct FindInfo {
@@ -1034,22 +1041,19 @@
// PRECONDITION: not an end() iterator.
reference operator*() const {
- ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
- "operator*() called on invalid iterator.");
+ ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator*()");
return PolicyTraits::element(slot_);
}
// PRECONDITION: not an end() iterator.
pointer operator->() const {
- ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
- "operator-> called on invalid iterator.");
+ ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator->");
return &operator*();
}
// PRECONDITION: not an end() iterator.
iterator& operator++() {
- ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
- "operator++ called on invalid iterator.");
+ ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator++");
++ctrl_;
++slot_;
skip_empty_or_deleted();
@@ -1081,7 +1085,7 @@
// Fixes up `ctrl_` to point to a full by advancing it and `slot_` until
// they reach one.
//
- // If a sentinel is reached, we null both of them out instead.
+ // If a sentinel is reached, we null `ctrl_` out instead.
void skip_empty_or_deleted() {
while (IsEmptyOrDeleted(*ctrl_)) {
uint32_t shift = Group{ctrl_}.CountLeadingEmptyOrDeleted();
@@ -1601,8 +1605,7 @@
// This overload is necessary because otherwise erase<K>(const K&) would be
// a better match if non-const iterator is passed as an argument.
void erase(iterator it) {
- ABSL_INTERNAL_ASSERT_IS_FULL(it.ctrl_,
- "erase() called on invalid iterator.");
+ ABSL_INTERNAL_ASSERT_IS_FULL(it.ctrl_, "erase()");
PolicyTraits::destroy(&alloc_ref(), it.slot_);
erase_meta_only(it);
}
@@ -1636,8 +1639,7 @@
}
node_type extract(const_iterator position) {
- ABSL_INTERNAL_ASSERT_IS_FULL(position.inner_.ctrl_,
- "extract() called on invalid iterator.");
+ ABSL_INTERNAL_ASSERT_IS_FULL(position.inner_.ctrl_, "extract()");
auto node =
CommonAccess::Transfer<node_type>(alloc_ref(), position.inner_.slot_);
erase_meta_only(position);
diff --git a/absl/container/internal/raw_hash_set_test.cc b/absl/container/internal/raw_hash_set_test.cc
index eec9da4..6478d3f 100644
--- a/absl/container/internal/raw_hash_set_test.cc
+++ b/absl/container/internal/raw_hash_set_test.cc
@@ -2036,20 +2036,59 @@
EXPECT_NE(old_ptr, addr(0));
}
-// Confirm that we assert if we try to erase() end().
-TEST(TableDeathTest, EraseOfEndAsserts) {
+bool IsAssertEnabled() {
// Use an assert with side-effects to figure out if they are actually enabled.
bool assert_enabled = false;
assert([&]() { // NOLINT
assert_enabled = true;
return true;
}());
- if (!assert_enabled) return;
+ return assert_enabled;
+}
+
+TEST(TableDeathTest, InvalidIteratorAsserts) {
+ if (!IsAssertEnabled()) GTEST_SKIP() << "Assertions not enabled.";
IntTable t;
// Extra simple "regexp" as regexp support is highly varied across platforms.
- constexpr char kDeathMsg[] = "erase.. called on invalid iterator";
- EXPECT_DEATH_IF_SUPPORTED(t.erase(t.end()), kDeathMsg);
+ EXPECT_DEATH_IF_SUPPORTED(
+ t.erase(t.end()),
+ "erase.* called on invalid iterator. The iterator might be an "
+ "end.*iterator or may have been default constructed.");
+ typename IntTable::iterator iter;
+ EXPECT_DEATH_IF_SUPPORTED(
+ ++iter,
+ "operator.* called on invalid iterator. The iterator might be an "
+ "end.*iterator or may have been default constructed.");
+ t.insert(0);
+ iter = t.begin();
+ t.erase(iter);
+ EXPECT_DEATH_IF_SUPPORTED(
+ ++iter,
+ "operator.* called on invalid iterator. The element might have been "
+ "erased or .*the table might have rehashed.");
+}
+
+TEST(TableDeathTest, IteratorInvalidAssertsEqualityOperator) {
+ if (!IsAssertEnabled()) GTEST_SKIP() << "Assertions not enabled.";
+
+ IntTable t;
+ t.insert(1);
+ t.insert(2);
+ t.insert(3);
+ auto iter1 = t.begin();
+ auto iter2 = std::next(iter1);
+ ASSERT_NE(iter1, t.end());
+ ASSERT_NE(iter2, t.end());
+ t.erase(iter1);
+ // Extra simple "regexp" as regexp support is highly varied across platforms.
+ const char* const kDeathMessage =
+ "Invalid operation on iterator. The element might have .*been erased or "
+ "the table might have rehashed.";
+ EXPECT_DEATH_IF_SUPPORTED(void(iter1 == iter2), kDeathMessage);
+ EXPECT_DEATH_IF_SUPPORTED(void(iter2 != iter1), kDeathMessage);
+ t.erase(iter2);
+ EXPECT_DEATH_IF_SUPPORTED(void(iter1 == iter2), kDeathMessage);
}
#if defined(ABSL_INTERNAL_HASHTABLEZ_SAMPLE)
