1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QKeyValueRange: make sure it is a viewable range

Browse Source 
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<Map<...> &>. 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<Map<...> &>
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 <thiago.macieira@intel.com>
This commit is contained in:
Giuseppe D'Angelo 2025-04-16 11:59:36 +02:00
parent 3a1f4f718d
commit aca7fd4ee7
4 changed files with 116 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/corelib/tools/qhash.h
View File

@ -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<QHash &>(*this); }
auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange<const QHash &>(*this); }
auto asKeyValueRange() && { return QtPrivate::QKeyValueRange<QHash>(std::move(*this)); }
auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange<QHash>(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<QMultiHash &>(*this); }
auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange<const QMultiHash &>(*this); }
auto asKeyValueRange() && { return QtPrivate::QKeyValueRange<QMultiHash>(std::move(*this)); }
auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange<QMultiHash>(std::move(*this)); }
iterator detach(const_iterator it)
{

29
src/corelib/tools/qiterator.h
View File

@ -7,6 +7,10 @@
#include <QtCore/qglobal.h>
#include <QtCore/qcontainertools_impl.h>
#ifdef __cpp_lib_ranges
#include <ranges>
#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 <typename Map>
class QKeyValueRangeStorage<Map &>
#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 <typename Map>
@ -299,18 +310,12 @@ class QKeyValueRange : public QKeyValueRangeStorage<Map>
{
public:
using QKeyValueRangeStorage<Map>::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 <typename Map>
QKeyValueRange(Map &) -> QKeyValueRange<Map &>;
template <typename Map, std::enable_if_t<!std::is_reference_v<Map>, bool> = false>
QKeyValueRange(Map &&) -> QKeyValueRange<std::remove_const_t<Map>>;
} // namespace QtPrivate

16
src/corelib/tools/qmap.h
View File

@ -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<QMap &>(*this); }
auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange<const QMap &>(*this); }
auto asKeyValueRange() && { return QtPrivate::QKeyValueRange<QMap>(std::move(*this)); }
auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange<QMap>(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<QMultiMap &>(*this); }
auto asKeyValueRange() const & { return QtPrivate::QKeyValueRange<const QMultiMap &>(*this); }
auto asKeyValueRange() && { return QtPrivate::QKeyValueRange<QMultiMap>(std::move(*this)); }
auto asKeyValueRange() const && { return QtPrivate::QKeyValueRange<QMultiMap>(std::move(*this)); }
iterator erase(const_iterator it)
{

83
tests/auto/corelib/tools/containerapisymmetry/tst_containerapisymmetry.cpp
View File

@ -1233,6 +1233,9 @@ void tst_ContainerApiSymmetry::member_erase_set_impl() const
QCOMPARE(it, c.cbegin());
}
template <typename T>
using KeyValueRangeType = decltype(std::declval<T>().asKeyValueRange());
template <typename Container>
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<std::remove_reference_t<decltype(value)>>);
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<std::remove_reference_t<decltype(value)>>);
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<std::remove_reference_t<decltype(value)>>);
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<const Container &&>(returnC()).asKeyValueRange()) {
static_assert(!std::is_const_v<std::remove_reference_t<decltype(value)>>); // 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<KeyValueRangeType<Container>>);
static_assert(std::ranges::viewable_range<KeyValueRangeType<Container &>>);
static_assert(std::ranges::viewable_range<KeyValueRangeType<const Container>>);
static_assert(std::ranges::viewable_range<KeyValueRangeType<const Container &>>);
static_assert(!std::ranges::view<KeyValueRangeType<Container>>);
static_assert(std::ranges::view<KeyValueRangeType<Container &>>);
static_assert(!std::ranges::view<KeyValueRangeType<const Container>>);
static_assert(std::ranges::view<KeyValueRangeType<const Container &>>);
const auto keyValueTest = [](auto &&pair)
{
return pair.first == pair.second - 2;
};
{
auto range = c.asKeyValueRange();
static_assert(std::ranges::view<decltype(range)>);
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<decltype(range)>);
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<decltype(range)>);
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<const Container &&>(returnC()).asKeyValueRange();
static_assert(!std::ranges::view<decltype(range)>);
QCOMPARE(std::ranges::distance(range), COUNT);
const bool ok = std::ranges::all_of(
range | std::views::transform(keyValueTest),
std::identity{}
);
QVERIFY(ok);
}
#endif
}
template<typename Container>