From aca7fd4ee764747bfa168f3e0f184babd3875b49 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Wed, 16 Apr 2025 11:59:36 +0200 Subject: [PATCH] QKeyValueRange: make sure it is a viewable range QMap/QHash::asKeyValueRange() returns a QKeyValueRange, which is a range, but in some cases it doesn't satisfy viewable_range: https://eel.is/c++draft/range.refinements#6 This causes reasonable code to fail to compile: map.asKeyValueRange() | std::views::transform(...) The reason for this is that, when called on a lvalue map, asKeyValueRange() returns QKeyValueRange &>. This isn't a viewable range: it's not a view itself, it's not a lvalue reference to range, and it's not movable. The last bit is actually a mistake, because QKeyValueRange &> contains a reference, and thus it's not assignable. Fix it by making it contain a pointer instead. In principle this has ABI implications because we're making the type POD (from non-POD), and this would allow usage of the tail padding on Itanium. I am 100% sure that no-one is inheriting from QKeyValueRange. We can also go a step further and decide to make QKeyValueRange a range *view*, because it is -- it satisfies all the semantic concepts. To elaborate, there are two different QKeyValueRange implementations, one for lvalue maps (stores a pointer to the map) and one for rvalue maps (moves the map into the range). Conceptually, they map to ref_view and owning_view in the standard library. For now I'm only making the QKeyValueRange<&> specialization for lvalue maps a view. The other one in principle may satisfy the range view requirements (as the containers are COW, so copy is actually O(1)) but this is more questionable. A possibility would be to make it move-only, but that's an API break. To make QKeyValueRange<&> a view I'm inheriting from view_base, which an empty class and thus doesn't affect ABI. (I'm not inheriting from view_interface in order to avoid changing the API between the two cases above.) This work unveiled two problems in the implementation of QKeyValueRange. The first was that the the deduction guides were actually unused for the const-lvalue argument. This is because user-defined deduction guides are added on top of the ones implictly generated by the compiler. The primary template constructor was used to synthesize a deduction guide that deduced a non-reference qualified T; the result was that a const lvalue map would yield a mutable range over a copy of the map (!). Fix this by getting rid of the deduction guides and explictly specifying the template arguments of QKeyValueRange. This fix in turn adds a problem for const rvalue maps (unlikely). The idea in this case would be to move the map into the QKeyValueRange: the range stores a map object, initialized by moving the source map. For a const map source, the stored map should be const as well. But that makes the range not movable (this is the point of things like movable-box in the std), and therefore not a view. Since this is a fringe case, I'm dropping the const for the inner stored map, so one can effectively realize a mutating iteration over a const rvalue source map. Task-number: QTBUG-105465 Change-Id: I70161029799376fd369e0332461a8a50e6062892 Reviewed-by: Thiago Macieira --- src/corelib/tools/qhash.h | 16 ++-- src/corelib/tools/qiterator.h | 29 ++++--- src/corelib/tools/qmap.h | 16 ++-- .../tst_containerapisymmetry.cpp | 83 +++++++++++++++++++ 4 files changed, 116 insertions(+), 28 deletions(-) diff --git a/src/corelib/tools/qhash.h b/src/corelib/tools/qhash.h index 478e9e78ce0..24c014e2153 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -1257,10 +1257,10 @@ public: inline const_key_value_iterator constKeyValueBegin() const noexcept { return const_key_value_iterator(begin()); } inline const_key_value_iterator keyValueEnd() const noexcept { return const_key_value_iterator(end()); } inline const_key_value_iterator constKeyValueEnd() const noexcept { return const_key_value_iterator(end()); } - auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } - auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } struct TryEmplaceResult { @@ -2179,10 +2179,10 @@ public: inline const_key_value_iterator constKeyValueBegin() const noexcept { return const_key_value_iterator(begin()); } inline const_key_value_iterator keyValueEnd() const noexcept { return const_key_value_iterator(end()); } inline const_key_value_iterator constKeyValueEnd() const noexcept { return const_key_value_iterator(end()); } - auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } - auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } iterator detach(const_iterator it) { diff --git a/src/corelib/tools/qiterator.h b/src/corelib/tools/qiterator.h index 8a2b493ef47..30b54951a55 100644 --- a/src/corelib/tools/qiterator.h +++ b/src/corelib/tools/qiterator.h @@ -7,6 +7,10 @@ #include #include +#ifdef __cpp_lib_ranges +#include +#endif + QT_BEGIN_NAMESPACE #if !defined(QT_NO_JAVA_STYLE_ITERATORS) @@ -280,6 +284,8 @@ class QKeyValueRangeStorage { protected: Map m_map; + Map &map() { return m_map; } + const Map &map() const { return m_map; } public: explicit QKeyValueRangeStorage(const Map &map) : m_map(map) {} explicit QKeyValueRangeStorage(Map &&map) : m_map(std::move(map)) {} @@ -287,11 +293,16 @@ public: template class QKeyValueRangeStorage +#ifdef __cpp_lib_ranges + : public std::ranges::view_base +#endif { protected: - Map &m_map; + Map *m_map; + Map &map() { return *m_map; } + const Map &map() const { return *m_map; } public: - explicit QKeyValueRangeStorage(Map &map) : m_map(map) {} + explicit QKeyValueRangeStorage(Map &map) : m_map(&map) {} }; template @@ -299,18 +310,12 @@ class QKeyValueRange : public QKeyValueRangeStorage { public: using QKeyValueRangeStorage::QKeyValueRangeStorage; - auto begin() { return this->m_map.keyValueBegin(); } - auto begin() const { return this->m_map.keyValueBegin(); } - auto end() { return this->m_map.keyValueEnd(); } - auto end() const { return this->m_map.keyValueEnd(); } + auto begin() { return this->map().keyValueBegin(); } + auto begin() const { return this->map().keyValueBegin(); } + auto end() { return this->map().keyValueEnd(); } + auto end() const { return this->map().keyValueEnd(); } }; -template -QKeyValueRange(Map &) -> QKeyValueRange; - -template , bool> = false> -QKeyValueRange(Map &&) -> QKeyValueRange>; - } // namespace QtPrivate diff --git a/src/corelib/tools/qmap.h b/src/corelib/tools/qmap.h index 15f41bfc426..01e614aa28b 100644 --- a/src/corelib/tools/qmap.h +++ b/src/corelib/tools/qmap.h @@ -620,10 +620,10 @@ public: const_key_value_iterator constKeyValueBegin() const { return const_key_value_iterator(begin()); } const_key_value_iterator keyValueEnd() const { return const_key_value_iterator(end()); } const_key_value_iterator constKeyValueEnd() const { return const_key_value_iterator(end()); } - auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } - auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } iterator erase(const_iterator it) { @@ -1353,10 +1353,10 @@ public: const_key_value_iterator constKeyValueBegin() const { return const_key_value_iterator(begin()); } const_key_value_iterator keyValueEnd() const { return const_key_value_iterator(end()); } const_key_value_iterator constKeyValueEnd() const { return const_key_value_iterator(end()); } - auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } - auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } - auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange(*this); } + auto asKeyValueRange() && { return QtPrivate::QKeyValueRange(std::move(*this)); } + auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange(std::move(*this)); } iterator erase(const_iterator it) { diff --git a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp index ecfcdea4cb7..3aa4085251d 100644 --- a/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp +++ b/tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp @@ -1233,6 +1233,9 @@ void tst_ContainerApiSymmetry::member_erase_set_impl() const QCOMPARE(it, c.cbegin()); } +template +using KeyValueRangeType = decltype(std::declval().asKeyValueRange()); + template void tst_ContainerApiSymmetry::keyValueRange_impl() const { @@ -1298,6 +1301,7 @@ void tst_ContainerApiSymmetry::keyValueRange_impl() const // auto &&, mutating keys.clear(); values.clear(); for (auto &&[key, value] : c.asKeyValueRange()) { + static_assert(!std::is_const_v>); keys << key; values << value; QCOMPARE(key, value - 1); @@ -1323,6 +1327,7 @@ void tst_ContainerApiSymmetry::keyValueRange_impl() const // auto &&, non-mutating (const map) keys.clear(); values.clear(); for (auto &&[key, value] : std::as_const(c).asKeyValueRange()) { + static_assert(std::is_const_v>); keys << key; values << value; QCOMPARE(key, value - 2); @@ -1345,6 +1350,7 @@ void tst_ContainerApiSymmetry::keyValueRange_impl() const // auto &&, non-mutating (rvalue map) keys.clear(); values.clear(); for (auto &&[key, value] : returnC().asKeyValueRange()) { + static_assert(!std::is_const_v>); keys << key; values << value; QCOMPARE(key, value - 2); @@ -1352,6 +1358,83 @@ void tst_ContainerApiSymmetry::keyValueRange_impl() const } QVERIFY(verify(keys, COUNT)); QVERIFY(verify(values, COUNT, 2)); + + // auto &&, non-mutating (const rvalue map) + keys.clear(); values.clear(); + for (auto &&[key, value] : const_cast(returnC()).asKeyValueRange()) { + static_assert(!std::is_const_v>); // non-const + keys << key; + values << value; + QCOMPARE(key, value - 2); + QCOMPARE(c.value(key), value); + } + QVERIFY(verify(keys, COUNT)); + QVERIFY(verify(values, COUNT, 2)); + +#if defined(__cpp_lib_ranges) && __cpp_lib_ranges > 202110L // P2415R2 + static_assert(std::ranges::viewable_range>); + static_assert(std::ranges::viewable_range>); + static_assert(std::ranges::viewable_range>); + static_assert(std::ranges::viewable_range>); + + static_assert(!std::ranges::view>); + static_assert(std::ranges::view>); + static_assert(!std::ranges::view>); + static_assert(std::ranges::view>); + + const auto keyValueTest = [](auto &&pair) + { + return pair.first == pair.second - 2; + }; + + { + auto range = c.asKeyValueRange(); + static_assert(std::ranges::view); + QCOMPARE(std::ranges::distance(range), COUNT); + + const bool ok = std::ranges::all_of( + range | std::views::transform(keyValueTest), + std::identity{} + ); + QVERIFY(ok); + } + + { + auto range = std::as_const(c).asKeyValueRange(); + static_assert(std::ranges::view); + QCOMPARE(std::ranges::distance(range), COUNT); + + const bool ok = std::ranges::all_of( + range | std::views::transform(keyValueTest), + std::identity{} + ); + QVERIFY(ok); + } + + { + auto range = returnC().asKeyValueRange(); + static_assert(!std::ranges::view); + QCOMPARE(std::ranges::distance(range), COUNT); + + const bool ok = std::ranges::all_of( + range | std::views::transform(keyValueTest), + std::identity{} + ); + QVERIFY(ok); + } + + { + auto range = const_cast(returnC()).asKeyValueRange(); + static_assert(!std::ranges::view); + QCOMPARE(std::ranges::distance(range), COUNT); + + const bool ok = std::ranges::all_of( + range | std::views::transform(keyValueTest), + std::identity{} + ); + QVERIFY(ok); + } +#endif } template