From ffb73175e6c5b35e6367c88479cc0bf160482016 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 4 Jun 2020 21:56:09 +0200 Subject: [PATCH] QVarLengthArray: add missing move special member functions A QVLA is copyable, so it should be movable, too. Added a helper function a la P1144's uninitialized_relocate_n to deal with the QTypeInfoQuery stuff. This way, the code is re-usable everywhere it's needed. The same cannot be said for QArrayDataOps, which only a parent can love... [ChangeLog][QtCore][QVarLengthArray] Added missing move constructor and move-assignment operator. Task-number: QTBUG-39111 Change-Id: If0dc2aa78eb29062d73dcd3dc4647ba345ae39e6 Reviewed-by: Thiago Macieira --- src/corelib/tools/qcontainertools_impl.h | 21 +++++ src/corelib/tools/qvarlengtharray.h | 41 +++++++++ src/corelib/tools/qvarlengtharray.qdoc | 11 +++ .../qvarlengtharray/tst_qvarlengtharray.cpp | 92 ++++++++++++++++--- 4 files changed, 150 insertions(+), 15 deletions(-) diff --git a/src/corelib/tools/qcontainertools_impl.h b/src/corelib/tools/qcontainertools_impl.h index 3a0c4381f12..b43c3bb1c4a 100644 --- a/src/corelib/tools/qcontainertools_impl.h +++ b/src/corelib/tools/qcontainertools_impl.h @@ -47,12 +47,33 @@ #define QCONTAINERTOOLS_IMPL_H #include +#include + #include +#include QT_BEGIN_NAMESPACE namespace QtPrivate { + +template +void q_uninitialized_relocate_n(T* first, N n, T* out) +{ + if constexpr (QTypeInfoQuery::isRelocatable) { + if (n != N(0)) { // even if N == 0, out == nullptr or first == nullptr are UB for memmove() + memmove(static_cast(out), + static_cast(first), + n * sizeof(T)); + } + } else { + std::uninitialized_move_n(first, n, out); + if constexpr (QTypeInfoQuery::isComplex) + std::destroy_n(first, n); + } +} + + template using IfIsInputIterator = typename std::enable_if< std::is_convertible::iterator_category, std::input_iterator_tag>::value, diff --git a/src/corelib/tools/qvarlengtharray.h b/src/corelib/tools/qvarlengtharray.h index f90e455fe48..cea0e14d41e 100644 --- a/src/corelib/tools/qvarlengtharray.h +++ b/src/corelib/tools/qvarlengtharray.h @@ -71,6 +71,26 @@ public: append(other.constData(), other.size()); } + QVarLengthArray(QVarLengthArray &&other) + noexcept(std::is_nothrow_move_constructible_v) + : a{other.a}, + s{other.s}, + ptr{other.ptr} + { + const auto otherInlineStorage = reinterpret_cast(other.array); + if (ptr == otherInlineStorage) { + // inline buffer - move into our inline buffer: + ptr = reinterpret_cast(array); + QtPrivate::q_uninitialized_relocate_n(otherInlineStorage, s, ptr); + } else { + // heap buffer - we just stole the memory + } + // reset other to internal storage: + other.a = Prealloc; + other.s = 0; + other.ptr = otherInlineStorage; + } + QVarLengthArray(std::initializer_list args) : QVarLengthArray(args.begin(), args.end()) { @@ -102,6 +122,27 @@ public: return *this; } + QVarLengthArray &operator=(QVarLengthArray &&other) + noexcept(std::is_nothrow_move_constructible_v) + { + // we're only required to be self-move-assignment-safe + // when we're in the moved-from state (Hinnant criterion) + // the moved-from state is the empty state, so we're good with the clear() here: + clear(); + Q_ASSERT(capacity() >= Prealloc); + const auto otherInlineStorage = reinterpret_cast(other.array); + if (other.ptr != otherInlineStorage) { + // heap storage: steal the external buffer, reset other to otherInlineStorage + a = std::exchange(other.a, Prealloc); + ptr = std::exchange(other.ptr, otherInlineStorage); + } else { + // inline storage: move into our storage (doesn't matter whether inline or external) + QtPrivate::q_uninitialized_relocate_n(other.ptr, other.s, ptr); + } + s = std::exchange(other.s, 0); + return *this; + } + QVarLengthArray &operator=(std::initializer_list list) { resize(qsizetype(list.size())); diff --git a/src/corelib/tools/qvarlengtharray.qdoc b/src/corelib/tools/qvarlengtharray.qdoc index e43d6f152a4..6371419ae5d 100644 --- a/src/corelib/tools/qvarlengtharray.qdoc +++ b/src/corelib/tools/qvarlengtharray.qdoc @@ -404,6 +404,12 @@ Assigns \a other to this array and returns a reference to this array. */ +/*! \fn template QVarLengthArray &QVarLengthArray::operator=(QVarLengthArray &&other) + Move-assigns \a other to this array and returns a reference to this array. + After the move, \a other is empty. + \since 6.0 + */ + /*! \fn template QVarLengthArray &QVarLengthArray::operator=(std::initializer_list list) \since 5.5 @@ -417,6 +423,11 @@ Constructs a copy of \a other. */ +/*! \fn template QVarLengthArray::QVarLengthArray(QVarLengthArray &&other) + Move-constructs this variable-length array from \a other. After the move, \a other is empty. + \since 6.0 + */ + /*! \fn template const T &QVarLengthArray::at(qsizetype i) const Returns a reference to the item at index position \a i. diff --git a/tests/auto/corelib/tools/qvarlengtharray/tst_qvarlengtharray.cpp b/tests/auto/corelib/tools/qvarlengtharray/tst_qvarlengtharray.cpp index 74654a38624..1ce5f1e0e69 100644 --- a/tests/auto/corelib/tools/qvarlengtharray/tst_qvarlengtharray.cpp +++ b/tests/auto/corelib/tools/qvarlengtharray/tst_qvarlengtharray.cpp @@ -29,14 +29,55 @@ #include #include #include +#include #include +struct Tracker +{ + static int count; + Tracker() { ++count; } + Tracker(const Tracker &) { ++count; } + Tracker(Tracker &&) { ++count; } + + Tracker &operator=(const Tracker &) = default; + Tracker &operator=(Tracker &&) = default; + + ~Tracker() { --count; } + +}; + +int Tracker::count = 0; + +template +class ValueTracker +{ + Tracker m_tracker; +public: + ValueTracker() = default; + ValueTracker(T value) : value{std::move(value)} {} + T value; + + friend bool operator==(const ValueTracker &lhs, const ValueTracker &rhs) noexcept + { return lhs.value == rhs.value; } + friend bool operator!=(const ValueTracker &lhs, const ValueTracker &rhs) noexcept + { return !operator==(lhs, rhs); } +}; + class tst_QVarLengthArray : public QObject { Q_OBJECT private slots: void append(); + void move_int_1() { move_int<1>(); } + void move_int_2() { move_int<2>(); } + void move_int_3() { move_int<3>(); } + void move_QString_1() { move_QString<1>(); } + void move_QString_2() { move_QString<2>(); } + void move_QString_3() { move_QString<3>(); } + void move_Tracker_1() { move_Tracker<1>(); } + void move_Tracker_2() { move_Tracker<2>(); } + void move_Tracker_3() { move_Tracker<3>(); } void removeLast(); void oldTests(); void appendCausingRealloc(); @@ -61,25 +102,18 @@ private slots: void implicitDefaultCtor(); private: + template + void move(T t1, T t2); + template + void move_int() { move(42, 24); } + template + void move_QString() { move("Hello", "World"); } + template + void move_Tracker(); template void initializeList(); }; -struct Tracker -{ - static int count; - Tracker() { ++count; } - Tracker(const Tracker &) { ++count; } - Tracker(Tracker &&) { ++count; } - - Tracker &operator=(const Tracker &) = default; - Tracker &operator=(Tracker &&) = default; - - ~Tracker() { --count; } -}; - -int Tracker::count = 0; - void tst_QVarLengthArray::append() { QVarLengthArray v; @@ -102,6 +136,34 @@ void tst_QVarLengthArray::append() v2.append(5); } +template +void tst_QVarLengthArray::move_Tracker() +{ + const auto reset = qScopeGuard([] { Tracker::count = 0; }); + move>({24}, {24}); + QCOMPARE(Tracker::count, 0); +} + +template +void tst_QVarLengthArray::move(T t1, T t2) +{ + { + QVarLengthArray v; + v.append(t1); + v.append(t2); + + auto moved = std::move(v); + QCOMPARE(moved.size(), 2); + QCOMPARE(moved[0], t1); + QCOMPARE(moved[1], t2); + + v = std::move(moved); + QCOMPARE(v.size(), 2); + QCOMPARE(v[0], t1); + QCOMPARE(v[1], t2); + } +} + void tst_QVarLengthArray::removeLast() { {