Sequential erase/_if: don't apply predicate twice to element

The code was trying to avoid a detach in the case no element needed to
be removed, by first running find_if() on const_iterators, and then,
after converting its result to (mutable) iterators, start the
remove_if() algorithm where find_if() left off.

But this applies the predicate to the element found by find_if() (if
any) _twice_: first just before we exit the first find_if() and then
just as we enter remove_if(), which will start by running find_if()
again, with the result of the initial find_if as 'first'.

Apart from being needlessly inefficient, this violates the
specification of Uniform Erasure, which defines sequential erase_if()
as being equivalent to remove_if() + container erase(), with the
former being specified to apply the predicate exactly once per
element.

Fix by writing the remove_if() part by hand.

Instead of doing the dance with the loop invariant documentation
twice, simply implement erase() via erase_if() (complicated a bit by
the weird passing of predicates by lvalue reference instead of by
value, as would be idiomatic). This exposes users to:

[ChangeLog][QtCore][Potentially Source-Incompatible Changes] A fix in
the implementation of the erase-like algorithms of sequential Qt
container may re-enable signed/unsigned comparison warnings previously
suppressed by having occurred in std library code. To fix, cast the
value to look for such that it has the same signedness as the
container's elements.

... but the issue would be the same had we inlined std::remove()
instead of passing a lambda to sequential_erase_if(), so it's nothing
we can, nor should, work around.

[ChangeLog][QtCore][Containers] Fixed a bug in the implementation of
most sequential Qt container's erase-like algorithms (member
removeAll()/removeIf() and free erase()/erase_if()) where the equality
operator or the predicate, respectively, was applied to the first
matching element twice. Each element is now tested exactly once.

Pick-to: 6.3 6.2
Change-Id: Ib6d24b01b40866c125406f1cd6042d4cd083ea0d
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>

src/corelib/tools/qcontainertools_impl.h

@@ -284,24 +284,51 @@ using IfIsNotSame =
 template<typename T, typename U>
 using IfIsNotConvertible = typename std::enable_if<!std::is_convertible<T, U>::value, bool>::type;
 
-template <typename Container, typename T>
-auto sequential_erase(Container &c, const T &t)
+template <typename Container, typename Predicate>
+auto sequential_erase_if(Container &c, Predicate &pred)
 {
-    // avoid a detach in case there is nothing to remove
+    // This is remove_if() modified to perform the find_if step on
+    // const_iterators to avoid shared container detaches if nothing needs to
+    // be removed. We cannot run remove_if after find_if: doing so would apply
+    // the predicate to the first matching element twice!
+
     const auto cbegin = c.cbegin();
     const auto cend = c.cend();
-    const auto t_it = std::find(cbegin, cend, t);
+    const auto t_it = std::find_if(cbegin, cend, pred);
     auto result = std::distance(cbegin, t_it);
     if (result == c.size())
         return result - result; // `0` of the right type
 
+    // now detach:
     const auto e = c.end();
-    const auto it = std::remove(std::next(c.begin(), result), e, t);
-    result = std::distance(it, e);
-    c.erase(it, e);
+
+    auto it = std::next(c.begin(), result);
+    auto dest = it;
+
+    // Loop Invariants:
+    // - it != e
+    // - [next(it), e[ still to be checked
+    // - [c.begin(), dest[ are result
+    while (++it != e) {
+        if (!pred(*it)) {
+            *dest = std::move(*it);
+            ++dest;
+        }
+    }
+
+    result = std::distance(dest, e);
+    c.erase(dest, e);
     return result;
 }
 
+template <typename Container, typename T>
+auto sequential_erase(Container &c, const T &t)
+{
+    // use the equivalence relation from http://eel.is/c++draft/list.erasure#1
+    auto cmp = [&](auto &e) { return e == t; };
+    return sequential_erase_if(c, cmp); // can't pass rvalues!
+}
+
 template <typename Container, typename T>
 auto sequential_erase_with_copy(Container &c, const T &t)
 {
@@ -321,24 +348,6 @@ auto sequential_erase_one(Container &c, const T &t)
     return true;
 }
 
-template <typename Container, typename Predicate>
-auto sequential_erase_if(Container &c, Predicate &pred)
-{
-    // avoid a detach in case there is nothing to remove
-    const auto cbegin = c.cbegin();
-    const auto cend = c.cend();
-    const auto t_it = std::find_if(cbegin, cend, pred);
-    auto result = std::distance(cbegin, t_it);
-    if (result == c.size())
-        return result - result; // `0` of the right type
-
-    const auto e = c.end();
-    const auto it = std::remove_if(std::next(c.begin(), result), e, pred);
-    result = std::distance(it, e);
-    c.erase(it, e);
-    return result;
-}
-
 template <typename T, typename Predicate>
 qsizetype qset_erase_if(QSet<T> &set, Predicate &pred)
 {

tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp

@@ -771,21 +771,35 @@ void tst_ContainerApiSymmetry::erase_if_impl() const
     auto c = make<Container>(7); // {1, 2, 3, 4, 5, 6, 7}
     QCOMPARE(c.size(), S(7));
 
-    auto result = erase_if(c, [](V i) { return Conv::toInt(i) % 2 == 0; });
+    decltype(c.size()) oldSize, count;
+
+    oldSize = c.size();
+    count = 0;
+    auto result = erase_if(c, [&](V i) { ++count; return Conv::toInt(i) % 2 == 0; });
     QCOMPARE(result, S(3));
     QCOMPARE(c.size(), S(4));
+    QCOMPARE(count, oldSize);
 
-    result = erase_if(c, [](V i) { return Conv::toInt(i) % 123 == 0; });
+    oldSize = c.size();
+    count = 0;
+    result = erase_if(c, [&](V i) { ++count; return Conv::toInt(i) % 123 == 0; });
     QCOMPARE(result, S(0));
     QCOMPARE(c.size(), S(4));
+    QCOMPARE(count, oldSize);
 
-    result = erase_if(c, [](V i) { return Conv::toInt(i) % 3 == 0; });
+    oldSize = c.size();
+    count = 0;
+    result = erase_if(c, [&](V i) { ++count; return Conv::toInt(i) % 3 == 0; });
     QCOMPARE(result, S(1));
     QCOMPARE(c.size(), S(3));
+    QCOMPARE(count, oldSize);
 
-    result = erase_if(c, [](V i) { return Conv::toInt(i) % 2 == 1; });
+    oldSize = c.size();
+    count = 0;
+    result = erase_if(c, [&](V i) { ++count; return Conv::toInt(i) % 2 == 1; });
     QCOMPARE(result, S(3));
     QCOMPARE(c.size(), S(0));
+    QCOMPARE(count, oldSize);
 }
 
 template <typename Container>