From 07d06324cba46ced30878afc6a1f591216f321b3 Mon Sep 17 00:00:00 2001 From: Jinghao Jia Date: Mon, 29 Jul 2024 10:57:29 -0700 Subject: [PATCH] fix off-by-one bug in FlatSet::difference_with 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 s1{1, 2, 3, 4}; sparta::FlatSet 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 --- include/sparta/FlatSet.h | 1 - test/SetTest.cpp | 5 +++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/include/sparta/FlatSet.h b/include/sparta/FlatSet.h index 1935102..baa5da0 100644 --- a/include/sparta/FlatSet.h +++ b/include/sparta/FlatSet.h @@ -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; } diff --git a/test/SetTest.cpp b/test/SetTest.cpp index 5fb8b04..5532812 100644 --- a/test/SetTest.cpp +++ b/test/SetTest.cpp @@ -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 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) {