[Development] qMoveToConst helper for rvalue references to movable Qt containers?

[Elvis Stansvik]

Den sön 21 okt. 2018 kl 16:15 skrev Elvis Stansvik <elvstone at gmail.com>:
>
> Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
> <development at qt-project.org>:
> >
> > Il 20/10/18 14:43, Elvis Stansvik ha scritto:
> > > In our application we added a helper like
> > >
> > > template <typename T>
> > > const T moveToConst(T &&t)
> > > {
> > >      return std::move(t);
> > > }
> > >
> > > that we use for these cases. It just moves the argument to a const
> > > value and returns it. With that we can do for (auto foo :
> > > moveToConst(returnsQtContainer()) { ... }.
> > >
> > > Since it's such a common pattern to want to iterate a returned Qt
> > > container, what would you think of having such a helper
> > > (qMoveToConst?) in Qt?
> >
> > Possibly... Note however that such a thing was already proposed for
> > qAsConst itself, and shut down to avoid having qAsConst diverge from
> > std::as_const (and I approve of not having Qt-specific behaviours). I
> > can't find the relevant discussion on the mailing list right now.
> >
> >
> > For reference, std::as_const's original authors had doubts about the
> > lifetime issues around this, saying that more investigations were needed:
> >
> > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html#9.0
> >
> >
> > After a LEWG meeting it was reworded like this:
> >
> > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r1.html#8.0
> >
> >
> > I'm not entirely sure of what prompted the change for as_const -- if
> > just a safety net while waiting for more investigation, or if something
> > more profound was raised.
> >
> >
> > I can easily imagine however a scenario where moveToConst() can lead to
> > worse code than the current solution.
> >
> > Current solution:
> >
> > // GCE may get applied, 0 moves
> > const QVector<Object> v = getVector();
> > for (auto &obj : v) ~~~
> >
> > vs.
> >
> > // Always 2 moves
> > for (auto &obj : qMoveToConst(getVector()) ~~~
> >
> >
> > And if the type is not even cheap to move (e.g. QVLA, std::array),
> > qMoveToConst becomes a really unpleasant surprise.

These two points of yours of course still stands, and I can understand
why you wouldn't want this as a helper in Qt because of it.

Elvis

> >
> > How about asking LEWG about their reasoning too?
>
> I couldn't find a way to contact them.
>
> In order to try out the unsafe usage you suggested in your other mail,
> and also another unsafe usage pointed out in an SO question
> (https://stackoverflow.com/questions/39051460/why-does-as-const-forbid-rvalue-arguments/39051612#39051612),
> I made the following test program.
>
> The output when running is:
>
> [estan at newton move-to-const-test]$ ./move-to-const-test
> without moveToConst:
> FooPrivate constr from vector
> Foo constr with arg 0x7fffdb627200
> Foo begin 0x7fffdb627200
> Foo end 0x7fffdb627200
> Foo destr 0x7fffdb627200
> FooPrivate destr
>
> with moveToConst:
> FooPrivate constr from vector
> Foo constr with arg 0x7fffdb627208
> Foo move constr 0x7fffdb627210
> Foo destr 0x7fffdb627208
> Foo begin const 0x7fffdb627210
> Foo end const 0x7fffdb627210
> Foo destr 0x7fffdb627210
> FooPrivate destr
> [estan at newton move-to-const-test]$
>
> Which just shows it's working as intended.
>
> The two unsafe usages are commented out, because they wouldn't compile (good!).
>
> Note that the version suggested under Further Discussion in rev 0 is
> different from our helper, we return std::move(t) while they return t.
>
> With the version from rev 0, your example unsafe usage will compile
> (the one from SO still won't).
>
> Elvis
>
> #include <QSharedData>
> #include <QSharedDataPointer>
> #include <QVector>
> #include <QtDebug>
>
> class FooPrivate : public QSharedData {
> public:
>     FooPrivate() {
>         qDebug() << "FooPrivate default constr";
>     };
>
>     FooPrivate(const FooPrivate &other) : QSharedData(other) {
>         qDebug() << "FooPrivate copy constr";
>     };
>
>     explicit FooPrivate(const QVector<int> &v) : v(v) {
>         qDebug() << "FooPrivate constr from vector";
>     };
>
>     ~FooPrivate() {
>         qDebug() << "FooPrivate destr";
>     };
>
>     QVector<int> v;
> };
>
> class Foo {
> public:
>     Foo() : d(new FooPrivate) {
>         qDebug() << "Foo default constr" << this;
>     };
>
>     Foo(const Foo &other) : d(other.d) {
>         qDebug() << "Foo copy constr" << this;
>     };
>
>     Foo(Foo &&other) : d(other.d) {
>         other.d = nullptr;
>         qDebug() << "Foo move constr" << this;
>     };
>
>     explicit Foo(const QVector<int> &v) : d(new FooPrivate(v)) {
>         qDebug() << "Foo constr with arg" << this;
>     };
>
>     ~Foo() {
>         qDebug() << "Foo destr" << this;
>     };
>
>     Foo &operator=(const Foo &other) {
>         qDebug() << "Foo copy ass op" << this;
>         d = other.d;
>         return *this;
>     };
>
>     QVector<int>::iterator begin() {
>         qDebug() << "Foo begin" << this;
>         return d->v.begin();
>     };
>
>     QVector<int>::const_iterator begin() const {
>         qDebug() << "Foo begin const" << this;
>         return d->v.begin();
>     };
>
>     QVector<int>::const_iterator cbegin() const {
>         qDebug() << "Foo cbegin" << this;
>         return d->v.cbegin();
>     };
>
>     QVector<int>::iterator end() {
>         qDebug() << "Foo end" << this;
>         return d->v.end();
>     };
>
>     QVector<int>::const_iterator end() const {
>         qDebug() << "Foo end const" << this;
>         return d->v.end();
>     };
>
>     QVector<int>::const_iterator cend() {
>         qDebug() << "Foo cend" << this;
>         return d->v.cend();
>     };
>
> private:
>     QSharedDataPointer<FooPrivate> d;
> };
>
> template <typename T>
> const T moveToConst(T &&t)
> {
>     return std::move(t);
> }
>
> Foo f() {
>     return Foo({1, 2, 3});
> }
>
> int main(int argc, char *argv[]) {
>     Q_UNUSED(argc);
>     Q_UNUSED(argv);
>
>     qDebug() << "without moveToConst:";
>     for (auto v : f()) { Q_UNUSED(v); }
>
>     qDebug() << "";
>
>     qDebug() << "with moveToConst:";
>     for (auto v : moveToConst(f())) { Q_UNUSED(v); }
>
>     /*
>      * Potential unsafe use suggested by Guiseppe D'Angelo
>      * https://lists.qt-project.org/pipermail/development/2018-October/033808.html
>      *
>      * Gives:
>      *
>      * move-to-const-test.cpp:93:23: error: cannot bind non-const
> lvalue reference of type ‘Foo&’ to an rvalue of type
> ‘std::remove_reference<Foo&>::type {aka Foo}’
>      *       return std::move(t);
>      *                         ^
>      */
>
>     //Foo baz;
>     //for (auto &v : moveToConst(baz)) { Q_UNUSED(v); };
>
>     /*
>      * Potential unsafe use from https://stackoverflow.com/a/39051612/252857
>      *
>      * Gives:
>      *
>      * move-to-const-test.cpp:118:42: error: cannot bind rvalue
> reference of type ‘const int&&’ to lvalue of type ‘const int’
>      *       for (auto const &&v : moveToConst(f())) { Q_UNUSED(v); }
> // whoops!
>      */
>
>     //for (auto const &&v : moveToConst(f())) { Q_UNUSED(v); }  // whoops!
>
>     return 0;
> }
>
> >
> >
> > My 2 c,
> > --
> > Giuseppe D'Angelo | giuseppe.dangelo at kdab.com | Senior Software Engineer
> > KDAB (France) S.A.S., a KDAB Group company
> > Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com
> > KDAB - The Qt, C++ and OpenGL Experts
> >
> > _______________________________________________
> > Development mailing list
> > Development at qt-project.org
> > http://lists.qt-project.org/mailman/listinfo/development