Skip to content

Commit 0465837

Browse files
ezbrcopybara-github
authored andcommitted
Improve error messages when comparing btree iterators.
- Add assertions that the iterators are either (a) from the same container or (b) both default constructed. Standard says: "The domain of == for forward iterators is that of iterators over the same underlying sequence. However, value-initialized iterators may be compared and shall compare equal to other value-initialized iterators of the same type." - [reference](https://eel.is/c++draft/forward.iterators#2). - Also optimize IsEndIterator a bit. PiperOrigin-RevId: 487617518 Change-Id: Iefba5c3bc8caa93954671793e6929e22f419c298
1 parent cc143ed commit 0465837

File tree

2 files changed

+58
-8
lines changed

2 files changed

+58
-8
lines changed

absl/container/btree_test.cc

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3358,6 +3358,31 @@ TEST(Btree, DereferencingEndIterator) {
33583358
EXPECT_DEATH(*set.end(), R"regex(Dereferencing end\(\) iterator)regex");
33593359
}
33603360

3361+
TEST(Btree, InvalidIteratorComparison) {
3362+
if (!IsAssertEnabled()) GTEST_SKIP() << "Assertions not enabled.";
3363+
3364+
absl::btree_set<int> set1, set2;
3365+
for (int i = 0; i < 1000; ++i) {
3366+
set1.insert(i);
3367+
set2.insert(i);
3368+
}
3369+
3370+
constexpr const char *kValueInitDeathMessage =
3371+
"Comparing default-constructed iterator with .*non-default-constructed "
3372+
"iterator";
3373+
typename absl::btree_set<int>::iterator iter1, iter2;
3374+
EXPECT_EQ(iter1, iter2);
3375+
EXPECT_DEATH(void(set1.begin() == iter1), kValueInitDeathMessage);
3376+
EXPECT_DEATH(void(iter1 == set1.begin()), kValueInitDeathMessage);
3377+
3378+
constexpr const char *kDifferentContainerDeathMessage =
3379+
"Comparing iterators from different containers";
3380+
iter1 = set1.begin();
3381+
iter2 = set2.begin();
3382+
EXPECT_DEATH(void(iter1 == iter2), kDifferentContainerDeathMessage);
3383+
EXPECT_DEATH(void(iter2 == iter1), kDifferentContainerDeathMessage);
3384+
}
3385+
33613386
} // namespace
33623387
} // namespace container_internal
33633388
ABSL_NAMESPACE_END

absl/container/internal/btree.h

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,6 +1017,17 @@ class btree_node {
10171017
friend struct btree_access;
10181018
};
10191019

1020+
template <typename Node>
1021+
bool AreNodesFromSameContainer(const Node *node_a, const Node *node_b) {
1022+
// If either node is null, then give up on checking whether they're from the
1023+
// same container. (If exactly one is null, then we'll trigger the
1024+
// default-constructed assert in Equals.)
1025+
if (node_a == nullptr || node_b == nullptr) return true;
1026+
while (!node_a->is_root()) node_a = node_a->parent();
1027+
while (!node_b->is_root()) node_b = node_b->parent();
1028+
return node_a == node_b;
1029+
}
1030+
10201031
template <typename Node, typename Reference, typename Pointer>
10211032
class btree_iterator {
10221033
using field_type = typename Node::field_type;
@@ -1073,16 +1084,16 @@ class btree_iterator {
10731084
}
10741085

10751086
bool operator==(const iterator &other) const {
1076-
return node_ == other.node_ && position_ == other.position_;
1087+
return Equals(other.node_, other.position_);
10771088
}
10781089
bool operator==(const const_iterator &other) const {
1079-
return node_ == other.node_ && position_ == other.position_;
1090+
return Equals(other.node_, other.position_);
10801091
}
10811092
bool operator!=(const iterator &other) const {
1082-
return node_ != other.node_ || position_ != other.position_;
1093+
return !Equals(other.node_, other.position_);
10831094
}
10841095
bool operator!=(const const_iterator &other) const {
1085-
return node_ != other.node_ || position_ != other.position_;
1096+
return !Equals(other.node_, other.position_);
10861097
}
10871098

10881099
// Returns n such that n calls to ++other yields *this.
@@ -1161,13 +1172,27 @@ class btree_iterator {
11611172
#endif
11621173
}
11631174

1175+
bool Equals(const node_type *other_node, int other_position) const {
1176+
ABSL_HARDENING_ASSERT(((node_ == nullptr && other_node == nullptr) ||
1177+
(node_ != nullptr && other_node != nullptr)) &&
1178+
"Comparing default-constructed iterator with "
1179+
"non-default-constructed iterator.");
1180+
// Note: we use assert instead of ABSL_HARDENING_ASSERT here because this
1181+
// changes the complexity of Equals from O(1) to O(log(N) + log(M)) where
1182+
// N/M are sizes of the containers containing node_/other_node.
1183+
assert(AreNodesFromSameContainer(node_, other_node) &&
1184+
"Comparing iterators from different containers.");
1185+
return node_ == other_node && position_ == other_position;
1186+
}
1187+
11641188
bool IsEndIterator() const {
11651189
if (position_ != node_->finish()) return false;
1166-
// Navigate to the rightmost node.
11671190
node_type *node = node_;
1168-
while (!node->is_root()) node = node->parent();
1169-
while (node->is_internal()) node = node->child(node->finish());
1170-
return node == node_;
1191+
while (!node->is_root()) {
1192+
if (node->position() != node->parent()->finish()) return false;
1193+
node = node->parent();
1194+
}
1195+
return true;
11711196
}
11721197

11731198
// Returns n such that n calls to ++other yields *this.

0 commit comments

Comments
 (0)