Merge pull request #1629 from mjbeardsley/unstable_remove_if_num_pred…

…_calls Ensure unstable_remove_if calls the predicate exactly once per element
ericniebler · Sep 12, 2021 · 9b07682 · 9b07682
2 parents 12067ce + 3877283
commit 9b07682
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 5 deletions.
diff --git a/include/range/v3/algorithm/unstable_remove_if.hpp b/include/range/v3/algorithm/unstable_remove_if.hpp
@@ -25,6 +25,7 @@
 #include <range/v3/algorithm/find_if_not.hpp>
 #include <range/v3/functional/identity.hpp>
 #include <range/v3/iterator/concepts.hpp>
+#include <range/v3/iterator/operations.hpp>
 #include <range/v3/iterator/reverse_iterator.hpp>
 #include <range/v3/range/access.hpp>
 #include <range/v3/range/concepts.hpp>
@@ -53,14 +54,18 @@ namespace ranges
             while(true)
             {
                 first = find_if(std::move(first), last, std::ref(pred), std::ref(proj));
-                last = find_if_not(make_reverse_iterator(std::move(last)),
-                                   make_reverse_iterator(first),
-                                   std::ref(pred),
-                                   std::ref(proj))
+                if(first == last)
+                    return first;
+
+                last = next(find_if_not(make_reverse_iterator(std::move(last)),
+                                        make_reverse_iterator(next(first)),
+                                        std::ref(pred),
+                                        std::ref(proj)))
                            .base();
                 if(first == last)
                     return first;
-                *first = iter_move(--last);
+
+                *first = iter_move(last);
 
                 // discussion here: https://github.com/ericniebler/range-v3/issues/988
                 ++first;

diff --git a/test/action/unstable_remove_if.cpp b/test/action/unstable_remove_if.cpp
@@ -106,6 +106,63 @@ void logic_test()
     }
 }
 
+void num_pred_calls_test()
+{
+    // std::ranges::remove_if requires:
+    // "Exactly N applications of the corresponding predicate and any projection, where N = (last - first)"
+    // https://en.cppreference.com/w/cpp/algorithm/ranges/remove
+    // so expect the same of unstable_remove_if
+    using namespace ranges;
+
+    int pred_invocation_counter = 0;
+    auto is_zero_count_invocations = [&pred_invocation_counter](int i) {
+        ++pred_invocation_counter;
+        return i == 0;
+    };
+
+    {
+        std::vector<int> vec{0};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 1);
+    }
+
+    {
+        std::vector<int> vec{1,1,1};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 3);
+    }
+
+    {
+        std::vector<int> vec{1,0};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 2);
+    }
+
+    {
+        std::vector<int> vec{1,2,0};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 3);
+    }
+
+    {
+        std::vector<int> vec{0,0,0,0};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 4);
+    }
+
+    {
+        std::vector<int> vec{1,2,3,0,0,0,0,4,5};
+        pred_invocation_counter = 0;
+        vec |= actions::unstable_remove_if(is_zero_count_invocations);
+        check_equal(pred_invocation_counter, 9);
+    }
+}
+
 class fuzzy_test_fn
 {
     int size;
@@ -209,6 +266,7 @@ class fuzzy_test_fn
 int main()
 {
     logic_test();
+    num_pred_calls_test();
 
     {
         const int size = 100;