From 6f5c78fe3d445f1c6c8738f9cedb9dbd847645fa Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 25 Feb 2022 08:47:48 +0100 Subject: [PATCH] QFlatMap: add remove_if The existing API of QFlatMap did not allow efficient removal of elements: - std::remove_if does not apply, because it works by moving elements back in the range onto those that need to be removed, which doesn't work in flat_map's case, because, like for all associative containers, the key in value_type is const. - The node-based erase-loop (over it = cond ? c.erase(it) : std::next(it)) works, but, unlike in traditional associative containers, is quadratic, because flat_map::erase is a linear operation. According to Stepanov's principle of Efficient Computational Basis (Elements of Programming, Section 1.4), we're therefore missing API. Add it. I couldn't make up my mind about the calling convention for the predicate and, despite having authored a merged paper about erase_if, can never remember what the predicate is supposed to take, so be fancy and accept all: (*it), (it.key(), it.value()), (it.key()). This means that unary predicates can either not be generic or must be properly constrained to distinguish between pair and K, but that's not necessarily a bad thing. There's no reason to supply a Qt-ified removeIf on top of the standard name, because this is private API and doubling the names would do nothing except double the testing overhead. Fixes: QTBUG-100983 Change-Id: I12545058958fc5d620baa770f92193c8de8b2d26 Reviewed-by: Edward Welbourne Reviewed-by: Ulf Hermann Reviewed-by: Qt CI Bot --- src/corelib/tools/qflatmap_p.h | 75 ++++++++++++++++ .../corelib/tools/qflatmap/tst_qflatmap.cpp | 87 +++++++++++++++++++ 2 files changed, 162 insertions(+) diff --git a/src/corelib/tools/qflatmap_p.h b/src/corelib/tools/qflatmap_p.h index 18d5dec652..7500cfb42d 100644 --- a/src/corelib/tools/qflatmap_p.h +++ b/src/corelib/tools/qflatmap_p.h @@ -863,6 +863,81 @@ public: return it; } + template + size_type remove_if(Predicate pred) + { + const auto indirect_call_to_pred = [pred = std::move(pred)](iterator it) { + auto dependent_false = [](auto &&...) { return false; }; + using Pair = decltype(*it); + using K = decltype(it.key()); + using V = decltype(it.value()); + using P = Predicate; + if constexpr (std::is_invocable_v) { + return pred(it.key(), it.value()); + } else if constexpr (std::is_invocable_v && !std::is_invocable_v) { + return pred(*it); + } else if constexpr (std::is_invocable_v && !std::is_invocable_v) { + return pred(it.key()); + } else { + static_assert(dependent_false(pred), + "Don't know how to call the predicate.\n" + "Options:\n" + "- pred(*it)\n" + "- pred(it.key(), it.value())\n" + "- pred(it.key())"); + } + }; + + auto first = begin(); + const auto last = end(); + + // find_if prefix loop + while (first != last && !indirect_call_to_pred(first)) + ++first; + + if (first == last) + return 0; // nothing to do + + // we know that we need to remove *first + + auto kdest = toKeysIterator(first); + auto vdest = toValuesIterator(first); + + ++first; + + auto k = std::next(kdest); + auto v = std::next(vdest); + + // Main Loop + // - first is used only for indirect_call_to_pred + // - operations are done on k, v + // Loop invariants: + // - first, k, v are pointing to the same element + // - [begin(), first[, [c.keys.begin(), k[, [c.values.begin(), v[: already processed + // - [first, end()[, [k, c.keys.end()[, [v, c.values.end()[: still to be processed + // - [c.keys.begin(), kdest[ and [c.values.begin(), vdest[ are keepers + // - [kdest, k[, [vdest, v[ are considered removed + // - kdest is not c.keys.end() + // - vdest is not v.values.end() + while (first != last) { + if (!indirect_call_to_pred(first)) { + // keep *first, aka {*k, *v} + *kdest = std::move(*k); + *vdest = std::move(*v); + ++kdest; + ++vdest; + } + ++k; + ++v; + ++first; + } + + const size_type r = std::distance(kdest, c.keys.end()); + c.keys.erase(kdest, c.keys.end()); + c.values.erase(vdest, c.values.end()); + return r; + } + key_compare key_comp() const noexcept { return static_cast(*this); diff --git a/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp b/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp index 8674734a11..ae6fcd6cad 100644 --- a/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp +++ b/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp @@ -40,6 +40,20 @@ #include #include +static constexpr bool is_even(int n) { return n % 2 == 0; } +static constexpr bool is_empty(QAnyStringView v) { return v.isEmpty(); } + +namespace { +template +constexpr inline bool is_pair_impl_v = false; +template +constexpr inline bool is_pair_impl_v> = true; +template +constexpr inline bool is_pair_v = is_pair_impl_v>; +template +using if_pair = std::enable_if_t, bool>; +} + class tst_QFlatMap : public QObject { Q_OBJECT @@ -53,6 +67,9 @@ private slots: void removal(); void extraction(); void iterators(); + void remove_if_pair() { remove_if_impl([](const auto &p) -> if_pair { return is_even(p.first) && is_empty(p.second); }); } + void remove_if_key_value() { remove_if_impl([](const auto &k, const auto &v) { return is_even(k) && is_empty(v); }); } + void remove_if_key() { remove_if_impl([](int k) { return is_even(k); }, true); } void statefulComparator(); void transparency_using(); void transparency_struct(); @@ -63,6 +80,8 @@ private slots: private: template void transparency_impl(); + template + void remove_if_impl(Predicate p, bool removeNonEmptyValues = false); }; #ifdef QFLATMAP_TEMPORARILY_REMOVED @@ -369,6 +388,74 @@ void tst_QFlatMap::iterators() } } +template +void tst_QFlatMap::remove_if_impl(Pred p, bool removeNonEmptyValues) +{ + // empty stays empty: + { + QFlatMap m; + QCOMPARE(m.remove_if(p), 0); + QVERIFY(m.isEmpty()); + } + // a matching element is removed: + { + { + QFlatMap m; + m.insert_or_assign(0, ""); + QCOMPARE(m.remove_if(p), 1); + QVERIFY(m.isEmpty()); + } + if (removeNonEmptyValues) { + QFlatMap m; + m.insert_or_assign(0, "x"); + QCOMPARE(m.remove_if(p), 1); + QVERIFY(m.isEmpty()); + } + } + // a non-matching element is not removed: + { + { + QFlatMap m; + m.insert_or_assign(1, ""); + QCOMPARE(m.remove_if(p), 0); + QVERIFY(m.contains(1)); + QVERIFY(m[1].isEmpty()); + } + if (removeNonEmptyValues) { + QFlatMap m; + m.insert_or_assign(1, "x"); + QCOMPARE(m.remove_if(p), 0); + QVERIFY(m.contains(1)); + QCOMPARE(m[1], "x"); + } + } + // of matching and non-matching elements, only matching ones are removed: + { + { + QFlatMap m; + m.insert_or_assign(0, ""); + m.insert_or_assign(1, ""); + const auto copy = m; + QCOMPARE(m.remove_if(p), 1); + QCOMPARE(copy.size(), 2); + QCOMPARE(copy[0], ""); + QCOMPARE(copy[1], ""); + QCOMPARE(m.size(), 1); + QVERIFY(m.contains(1)); + QVERIFY(m[1].isEmpty()); + } + { + QFlatMap m; + m.insert_or_assign(1, ""); + m.insert_or_assign(2, ""); + QCOMPARE(m.remove_if(p), 1); + QCOMPARE(m.size(), 1); + QVERIFY(m.contains(1)); + QVERIFY(m[1].isEmpty()); + } + } +} + void tst_QFlatMap::removal() { #ifdef QFLATMAP_TEMPORARILY_REMOVED