From 9f31f579ec2e6f15c6c87b0e3c5c762ea2690bc9 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Mon, 31 Jan 2022 23:42:49 +0100 Subject: [PATCH] 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 --- src/corelib/tools/qcontainertools_impl.h | 59 +++++++++++-------- .../tst_containerapisymmetry.cpp | 22 +++++-- 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/corelib/tools/qcontainertools_impl.h b/src/corelib/tools/qcontainertools_impl.h index 723c984d81..335a193144 100644 --- a/src/corelib/tools/qcontainertools_impl.h +++ b/src/corelib/tools/qcontainertools_impl.h @@ -284,24 +284,51 @@ using IfIsNotSame = template using IfIsNotConvertible = typename std::enable_if::value, bool>::type; -template -auto sequential_erase(Container &c, const T &t) +template +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 +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 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 -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 qsizetype qset_erase_if(QSet &set, Predicate &pred) { diff --git a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp index 7fe1dd9b4d..95d2076b6e 100644 --- a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp +++ b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp @@ -771,21 +771,35 @@ void tst_ContainerApiSymmetry::erase_if_impl() const auto c = make(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