Skip to content

Commit

Permalink
fix off-by-one bug in FlatSet::difference_with
Browse files Browse the repository at this point in the history
Summary:
The current implementation of `FlatSet::difference_with()` performs an
increment on the iterator returned by `m_set.erase()`. Since `m_set.erase()`
will move the iterator to the next element after the element being erased,
incrementing the iterator again creates an off-by-one bug, which manifests if
consecutive elements in `this` is also in `other`. Consequently we have the
following incorrect behavior:

```
sparta::FlatSet<int> s1{1, 2, 3, 4};
sparta::FlatSet<int> s2{1, 2, 3, 4};
std::cerr << s1.get_difference_with(s2) << std::endl; // expected {}, got {2, 4}
```

Reviewed By: anwesht, arnaudvenet

Differential Revision: D60269360

fbshipit-source-id: 7bb9a19eeab42e98bf2fd45b583ce5bcd21a8dd7
  • Loading branch information
jinghao-jia authored and facebook-github-bot committed Jul 29, 2024
1 parent 2e20749 commit 07d0632
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 1 deletion.
1 change: 0 additions & 1 deletion include/sparta/FlatSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ class FlatSet final
if (it != end && Equal()(*it, *other_it)) {
it = m_set.erase(it);
end = m_set.end();
++it;
}
++other_it;
}
Expand Down
5 changes: 5 additions & 0 deletions test/SetTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,11 @@ TYPED_TEST(UInt32SetTest, basicOperations) {
TypeParam d43 = t4.get_difference_with(t3);
EXPECT_THAT(d43,
::testing::UnorderedElementsAre(0, 1, 5, 101, 8137, 1234567));

std::vector<uint32_t> elements5 = {1, 2, 1023, 4096, 13001, bigint};
TypeParam t5(elements5.begin(), elements5.end());
TypeParam d54 = t5.get_difference_with(t4);
EXPECT_THAT(d54, ::testing::UnorderedElementsAre(1023, 13001));
}

TYPED_TEST(UInt32SetTest, robustness) {
Expand Down

0 comments on commit 07d0632

Please sign in to comment.