From 1977c922e9dc8a5402ca056faa68014f87275c18 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 25 Jan 2022 13:51:28 +0100 Subject: [PATCH] QFlatMap: make insertion STL-compatible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That is, insert() doesn't overwrite an existing entry, and range insert inserts the first of equivalent keys' values, not the last. This allowed this author to optimize the implementation of makeUnique() to a O(N) algorithm (was: O(N²)). Said optimization would have been possible with the old semantics, too, but I wrote the algorithm first and only then noticed the broken insert() behavior is present on QFlatMap, too, so I decided not to let good code go to waste and to fix both problems at the same time. In order to give users a hint of the changed semantics, make the new API opt-in until Qt 6.5, so Qt 6.4 ships with the both the old and the new semantics disabled, where they contradict. Fixes: QTBUG-100092 Change-Id: Ic96d8bfe6bed9068dbe8c0d7171bd8921050fd95 Reviewed-by: Qt CI Bot Reviewed-by: Mårten Nordheim Reviewed-by: Fabian Kosmale --- src/corelib/tools/qflatmap_p.h | 74 +++++++++++++------ .../corelib/tools/qflatmap/tst_qflatmap.cpp | 19 +---- 2 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/corelib/tools/qflatmap_p.h b/src/corelib/tools/qflatmap_p.h index 807a68ffaa..2cabb5873e 100644 --- a/src/corelib/tools/qflatmap_p.h +++ b/src/corelib/tools/qflatmap_p.h @@ -78,6 +78,19 @@ QT_BEGIN_NAMESPACE QFlatMap, std::vector, std::vector> */ +// Qt 6.4: +// - removed QFlatMap API which was incompatible with STL semantics +// - will be released with said API disabled, to catch any out-of-tree users +// - also allows opting in to the new API using QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT +// Qt 6.5 +// - will make QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT the default: + +#ifndef QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT +# if QT_VERSION >= QT_VERSION_CHECK(6, 5, 0) +# define QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT +# endif +#endif + namespace Qt { struct OrderedUniqueRange_t {}; @@ -431,7 +444,7 @@ private: public: QFlatMap() = default; -#ifdef QFLATMAP_TEMPORARILY_REMOVED +#ifdef QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT explicit QFlatMap(const key_container_type &keys, const mapped_container_type &values) : c{keys, values} { @@ -509,7 +522,7 @@ public: { } -#ifdef QFLATMAP_TEMPORARILY_REMOVED +#ifdef QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT explicit QFlatMap(const key_container_type &keys, const mapped_container_type &values, const Compare &compare) : value_compare(compare), c{keys, values} @@ -690,25 +703,25 @@ public: return value(key); } -#ifdef QFLATMAP_TEMPORARILY_REMOVED +#ifdef QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT std::pair insert(const Key &key, const T &value) { - return insert_or_assign(key, value); + return try_emplace(key, value); } std::pair insert(Key &&key, const T &value) { - return insert_or_assign(std::move(key), value); + return try_emplace(std::move(key), value); } std::pair insert(const Key &key, T &&value) { - return insert_or_assign(key, std::move(value)); + return try_emplace(key, std::move(value)); } std::pair insert(Key &&key, T &&value) { - return insert_or_assign(std::move(key), std::move(value)); + return try_emplace(std::move(key), std::move(value)); } #endif @@ -754,7 +767,7 @@ public: return r; } -#ifdef QFLATMAP_TEMPORARILY_REMOVED +#ifdef QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT template = nullptr> void insert(InputIt first, InputIt last) { @@ -1078,24 +1091,39 @@ private: void makeUnique() { - if (c.keys.size() < 2) + // std::unique, but over two ranges + auto equivalent = [this](const auto &lhs, const auto &rhs) { + return !key_compare::operator()(lhs, rhs) && !key_compare::operator()(rhs, lhs); + }; + const auto kb = c.keys.begin(); + const auto ke = c.keys.end(); + auto k = std::adjacent_find(kb, ke, equivalent); + if (k == ke) return; - auto k = std::end(c.keys) - 1; - auto i = k - 1; - for (;;) { - if (key_compare::operator()(*i, *k) || key_compare::operator()(*k, *i)) { - if (i == std::begin(c.keys)) - break; - --i; - --k; - } else { - c.values.erase(std::begin(c.values) + std::distance(std::begin(c.keys), i)); - i = c.keys.erase(i); - if (i == std::begin(c.keys)) - break; - k = i + 1; + + // equivalent keys found, we need to do actual work: + auto v = std::next(c.values.begin(), std::distance(kb, k)); + + auto kdest = k; + auto vdest = v; + + ++k; + ++v; + + // Loop Invariants: + // + // - [keys.begin(), kdest] and [values.begin(), vdest] are unique + // - k is not keys.end(), v is not values.end() + // - [next(k), keys.end()[ and [next(v), values.end()[ still need to be checked + while ((++v, ++k) != ke) { + if (!equivalent(*kdest, *k)) { + *++kdest = std::move(*k); + *++vdest = std::move(*v); } } + + c.keys.erase(std::next(kdest), ke); + c.values.erase(std::next(vdest), c.values.end()); } containers c; diff --git a/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp b/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp index 2dd6c16f13..ed46a4546a 100644 --- a/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp +++ b/tests/auto/corelib/tools/qflatmap/tst_qflatmap.cpp @@ -27,6 +27,7 @@ ****************************************************************************/ #define QT_USE_QSTRINGBUILDER +#define QFLATMAP_ENABLE_STL_COMPATIBLE_INSERT #include @@ -58,11 +59,9 @@ class tst_QFlatMap : public QObject { Q_OBJECT private slots: -#ifdef QFLATMAP_TEMPORARILY_REMOVED void constructing(); void constAccess(); void insertion(); -#endif void insertRValuesAndLValues(); void removal(); void extraction(); @@ -84,7 +83,6 @@ private: void remove_if_impl(Predicate p, bool removeNonEmptyValues = false); }; -#ifdef QFLATMAP_TEMPORARILY_REMOVED void tst_QFlatMap::constructing() { using Map = QFlatMap; @@ -169,7 +167,7 @@ void tst_QFlatMap::insertion() QCOMPARE(m.value("foo").data(), "FOO"); QCOMPARE(m.value("bar").data(), "BAR"); QCOMPARE(m.value("baz").data(), "BAZ"); - QCOMPARE(m.value("oof").data(), "OOF"); + QCOMPARE(m.value("oof").data(), "eek"); QCOMPARE(m.value("bla").data(), "BLA"); QCOMPARE(m.value("blubb").data(), "BLUBB"); @@ -183,14 +181,12 @@ void tst_QFlatMap::insertion() m.insert(std::begin(a1), std::end(a1)); m.insert(Qt::OrderedUniqueRange, std::begin(a2), std::end(a2)); QCOMPARE(m.size(), 10); - QCOMPARE(m.value("narf").data(), "NARFFFFFF"); + QCOMPARE(m.value("narf").data(), "NARF"); QCOMPARE(m.value("gnampf").data(), "GNAMPF"); } -#endif void tst_QFlatMap::insertRValuesAndLValues() { -#ifdef QFLATMAP_TEMPORARILY_REMOVED using Map = QFlatMap; const QByteArray foo = QByteArrayLiteral("foo"); const QByteArray bar = QByteArrayLiteral("bar"); @@ -223,7 +219,6 @@ void tst_QFlatMap::insertRValuesAndLValues() } #undef lvalue -#endif } void tst_QFlatMap::extraction() @@ -458,7 +453,6 @@ void tst_QFlatMap::remove_if_impl(Pred p, bool removeNonEmptyValues) void tst_QFlatMap::removal() { -#ifdef QFLATMAP_TEMPORARILY_REMOVED using Map = QFlatMap; Map m({ { 2, "bar" }, { 3, "baz" }, { 1, "foo" } }); QCOMPARE(m.value(2).data(), "bar"); @@ -481,12 +475,10 @@ void tst_QFlatMap::removal() it = m.erase(it); QCOMPARE(it.key(), 2); QVERIFY(!m.contains(1)); -#endif } void tst_QFlatMap::statefulComparator() { -#ifdef QFLATMAP_TEMPORARILY_REMOVED struct CountingCompare { mutable int count = 0; @@ -504,7 +496,6 @@ void tst_QFlatMap::statefulComparator() QCOMPARE(m2.key_comp().count, m1.key_comp().count); m2.insert(m1.begin(), m1.end()); QVERIFY(m2.key_comp().count > m1.key_comp().count); -#endif } void tst_QFlatMap::transparency_using() @@ -536,7 +527,6 @@ void tst_QFlatMap::transparency_struct() template void tst_QFlatMap::transparency_impl() { -#ifdef QFLATMAP_TEMPORARILY_REMOVED using Map = QFlatMap; auto m = Map{ { "one", "een" }, { "two", "twee" }, { "three", "dree" } }; @@ -560,7 +550,6 @@ void tst_QFlatMap::transparency_impl() QVERIFY(m.contains(QLatin1String("one"))); QVERIFY(m.remove(QAnyStringView(u8"one"))); QVERIFY(!m.contains(QLatin1String("one"))); -#endif } void tst_QFlatMap::try_emplace_and_insert_or_assign() @@ -707,7 +696,6 @@ void tst_QFlatMap::try_emplace_and_insert_or_assign() void tst_QFlatMap::viewIterators() { -#ifdef QFLATMAP_TEMPORARILY_REMOVED using Map = QFlatMap; Map m({ { "yksi", "een"}, { "kaksi", "twee" }, { "kolme", "dree" } }); { @@ -752,7 +740,6 @@ void tst_QFlatMap::viewIterators() it--; QCOMPARE(*it, "dree"); } -#endif } void tst_QFlatMap::varLengthArray()