Skip to content

Commit

Permalink
QVarLengthArray: add missing move special member functions
Browse files Browse the repository at this point in the history
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 <thiago.macieira@intel.com>
  • Loading branch information
marc-kdab committed Jun 22, 2020
1 parent 670c5bd commit ffb7317
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 15 deletions.
21 changes: 21 additions & 0 deletions src/corelib/tools/qcontainertools_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,33 @@
#define QCONTAINERTOOLS_IMPL_H

#include <QtCore/qglobal.h>
#include <QtCore/qtypeinfo.h>

#include <iterator>
#include <memory>

QT_BEGIN_NAMESPACE

namespace QtPrivate
{

template <typename T, typename N>
void q_uninitialized_relocate_n(T* first, N n, T* out)
{
if constexpr (QTypeInfoQuery<T>::isRelocatable) {
if (n != N(0)) { // even if N == 0, out == nullptr or first == nullptr are UB for memmove()
memmove(static_cast<void*>(out),
static_cast<const void*>(first),
n * sizeof(T));
}
} else {
std::uninitialized_move_n(first, n, out);
if constexpr (QTypeInfoQuery<T>::isComplex)
std::destroy_n(first, n);
}
}


template <typename Iterator>
using IfIsInputIterator = typename std::enable_if<
std::is_convertible<typename std::iterator_traits<Iterator>::iterator_category, std::input_iterator_tag>::value,
Expand Down
41 changes: 41 additions & 0 deletions src/corelib/tools/qvarlengtharray.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,26 @@ class QVarLengthArray
append(other.constData(), other.size());
}

QVarLengthArray(QVarLengthArray &&other)
noexcept(std::is_nothrow_move_constructible_v<T>)
: a{other.a},
s{other.s},
ptr{other.ptr}
{
const auto otherInlineStorage = reinterpret_cast<T*>(other.array);
if (ptr == otherInlineStorage) {
// inline buffer - move into our inline buffer:
ptr = reinterpret_cast<T*>(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<T> args)
: QVarLengthArray(args.begin(), args.end())
{
Expand Down Expand Up @@ -102,6 +122,27 @@ class QVarLengthArray
return *this;
}

QVarLengthArray &operator=(QVarLengthArray &&other)
noexcept(std::is_nothrow_move_constructible_v<T>)
{
// 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<T*>(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<T, Prealloc> &operator=(std::initializer_list<T> list)
{
resize(qsizetype(list.size()));
Expand Down
11 changes: 11 additions & 0 deletions src/corelib/tools/qvarlengtharray.qdoc
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,12 @@
Assigns \a other to this array and returns a reference to this array.
*/

/*! \fn template<class T, qsizetype Prealloc> QVarLengthArray<T, Prealloc> &QVarLengthArray<T, Prealloc>::operator=(QVarLengthArray<T, Prealloc> &&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<class T, qsizetype Prealloc> QVarLengthArray<T, Prealloc> &QVarLengthArray<T, Prealloc>::operator=(std::initializer_list<T> list)
\since 5.5

Expand All @@ -417,6 +423,11 @@
Constructs a copy of \a other.
*/

/*! \fn template<class T, qsizetype Prealloc> QVarLengthArray<T, Prealloc>::QVarLengthArray(QVarLengthArray<T, Prealloc> &&other)
Move-constructs this variable-length array from \a other. After the move, \a other is empty.
\since 6.0
*/

/*! \fn template<class T, qsizetype Prealloc> const T &QVarLengthArray<T, Prealloc>::at(qsizetype i) const

Returns a reference to the item at index position \a i.
Expand Down
92 changes: 77 additions & 15 deletions tests/auto/corelib/tools/qvarlengtharray/tst_qvarlengtharray.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,55 @@
#include <QtTest/QtTest>
#include <qvarlengtharray.h>
#include <qvariant.h>
#include <qscopeguard.h>

#include <memory>

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 <typename T>
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();
Expand All @@ -61,25 +102,18 @@ private slots:
void implicitDefaultCtor();

private:
template <qsizetype N, typename T>
void move(T t1, T t2);
template <qsizetype N>
void move_int() { move<N, int>(42, 24); }
template <qsizetype N>
void move_QString() { move<N, QString>("Hello", "World"); }
template <qsizetype N>
void move_Tracker();
template<typename T>
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<QString, 2> v;
Expand All @@ -102,6 +136,34 @@ void tst_QVarLengthArray::append()
v2.append(5);
}

template <qsizetype N>
void tst_QVarLengthArray::move_Tracker()
{
const auto reset = qScopeGuard([] { Tracker::count = 0; });
move<N, ValueTracker<int>>({24}, {24});
QCOMPARE(Tracker::count, 0);
}

template <qsizetype N, typename T>
void tst_QVarLengthArray::move(T t1, T t2)
{
{
QVarLengthArray<T, N> 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()
{
{
Expand Down

0 comments on commit ffb7317

Please sign in to comment.