Discussion:
[Development] qMoveToConst helper for rvalue references to movable Qt containers?
Elvis Stansvik
2018-10-20 12:43:45 UTC
Permalink
Hi all (first post),

In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.

For good reasons there's no version of qAsConst that takes an rvalue
reference, so you can't do e.g. for (auto foo :
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.

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?

Cheers,
Elvis
Elvis Stansvik
2018-10-20 17:37:26 UTC
Permalink
Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
Post by Elvis Stansvik
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
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
Post by Elvis Stansvik
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html#9.0
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.
// 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.
How about asking LEWG about their reasoning too?
Thanks a lot for the pointers and back story Giuseppe. I definitely
think it's good that qAsConst mirrors what std::as_const did, so
wouldn't want it expanded.

If the C++ wizards considered this but were hesitant, then I think
it's right that Qt is hesitant as well. I hadn't really considered
potential life-time issues, so I guess what we're doing might possibly
be unsafe (?).

What's GCE? Some optimization? (Google "c++ gce" didn't give me anything).

Elvis
My 2 c,
--
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
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-20 21:58:53 UTC
Permalink
(Adding back the mailing list)
Den lör 20 okt. 2018 kl 23:50 skrev Giuseppe D'Angelo
Post by Elvis Stansvik
If the C++ wizards considered this but were hesitant, then I think
it's right that Qt is hesitant as well. I hadn't really considered
potential life-time issues, so I guess what we're doing might possibly
be unsafe (?).
QVector<Obj> v;
for (auto &o : qMoveAsConst(v)) ~~~; // compiles with an lvalue!
use(v); // unspecified behavior
Ah yes, it may be that the standards folks simply didn't want this
because of foot-shooting potential like this.
Another potential foot-shooter I found on an SO question
(https://stackoverflow.com/a/39051612/252857):

for (auto const &&value : as_const(getQString())) // whoops!
{
}

Elvis
Post by Elvis Stansvik
What's GCE? Some optimization? (Google "c++ gce" didn't give me anything).
https://en.cppreference.com/w/cpp/language/copy_elision
Thanks!
Elvis
My 2 c,
--
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
Elvis Stansvik
2018-10-21 14:15:58 UTC
Permalink
Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
Post by Elvis Stansvik
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
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
Post by Elvis Stansvik
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html#9.0
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.
// 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.
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:

[***@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
[***@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,
--
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
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-21 14:20:22 UTC
Permalink
Post by Elvis Stansvik
Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
Post by Elvis Stansvik
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
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
Post by Elvis Stansvik
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html#9.0
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.
// 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
Post by Elvis Stansvik
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.
FooPrivate constr from vector
Foo constr with arg 0x7fffdb627200
Foo begin 0x7fffdb627200
Foo end 0x7fffdb627200
Foo destr 0x7fffdb627200
FooPrivate destr
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
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 {
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 {
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();
};
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
*
*
* 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
*
*
* 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,
--
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
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-21 16:23:35 UTC
Permalink
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
Hello,
Post by Elvis Stansvik
I couldn't find a way to contact them.
The best shot would be the std-discussion mailing list, I think.
Post by Elvis Stansvik
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.
FooPrivate constr from vector
Foo constr with arg 0x7fffdb627200
Foo begin 0x7fffdb627200
Foo end 0x7fffdb627200
Foo destr 0x7fffdb627200
FooPrivate destr
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
Which just shows it's working as intended.
However the third test should be a without moveToConst, but storing in a
temporary, i.e. the current best practice. It should output exactly like
the first one, but of course call const begin/end.
And note the extra move/destruciton in the second example.
Yep, well aware of the extra move/destruction. It's the price we pay
for saving that extra line of code to define a const variable.
Post by Elvis Stansvik
The two unsafe usages are commented out, because they wouldn't compile (good!).
template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}
This won't compile when called on a non-const lvalue (the return type
will be a lvalue reference to const, which won't bind to a non-const
rvalue). I stand corrected.
Which actually makes me think of yet another possibility of misuse, that
is, applying moveToConst to a function returning a const object. That
will compile, using a copy...
Post by Elvis Stansvik
const QVector<Obj> getVector();
for (auto &obj : moveToConst(getVector()) ~~~
Yes, that would be useless, but at least not UB :)
Long story short, I think it's good if we stay away from this in Qt.
Clazy warns you if you "do it wrong", and being a Qt-specific problem,
we better not offer too many ways that could have unpleasant drawbacks.
Yea, I can understand that. And since all it saves is a line of code,
I guess not worth the trouble.

Elvis
My 2 c,
--
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
Elvis Stansvik
2018-10-21 16:45:58 UTC
Permalink
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
Hello,
Post by Elvis Stansvik
I couldn't find a way to contact them.
The best shot would be the std-discussion mailing list, I think.
Post by Elvis Stansvik
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.
FooPrivate constr from vector
Foo constr with arg 0x7fffdb627200
Foo begin 0x7fffdb627200
Foo end 0x7fffdb627200
Foo destr 0x7fffdb627200
FooPrivate destr
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
Which just shows it's working as intended.
However the third test should be a without moveToConst, but storing in a
temporary, i.e. the current best practice. It should output exactly like
the first one, but of course call const begin/end.
For completeness sake, indeed

qDebug() << "with recommended way:";
const auto stuff = f();
for (auto v : stuff) { Q_UNUSED(v); }

gives

with recommended way:
FooPrivate constr from vector
Foo constr with arg 0x7ffdc9d50040
Foo begin const 0x7ffdc9d50040
Foo end const 0x7ffdc9d50040
Foo destr 0x7ffdc9d50040
FooPrivate destr

as expected.

Elvis
And note the extra move/destruciton in the second example.
Post by Elvis Stansvik
The two unsafe usages are commented out, because they wouldn't compile (good!).
template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}
This won't compile when called on a non-const lvalue (the return type
will be a lvalue reference to const, which won't bind to a non-const
rvalue). I stand corrected.
Which actually makes me think of yet another possibility of misuse, that
is, applying moveToConst to a function returning a const object. That
will compile, using a copy...
Post by Elvis Stansvik
const QVector<Obj> getVector();
for (auto &obj : moveToConst(getVector()) ~~~
Long story short, I think it's good if we stay away from this in Qt.
Clazy warns you if you "do it wrong", and being a Qt-specific problem,
we better not offer too many ways that could have unpleasant drawbacks.
My 2 c,
--
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
André Pönitz
2018-10-22 19:40:29 UTC
Permalink
Post by Elvis Stansvik
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.
FooPrivate constr from vector
Foo constr with arg 0x7fffdb627200
Foo begin 0x7fffdb627200
Foo end 0x7fffdb627200
Foo destr 0x7fffdb627200
FooPrivate destr
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
Which just shows it's working as intended.
I have (a) no example that triggers obviously bad behaviour and (b)
a bad gut feeling nevertheless.

The problem is that a 'move' could be a 'swap' in practice, with the
to-be-destroyed object held 'for a while', effectively turning C++'s
rather deterministic destruction behaviour into something resembling
garbage collection.

I wouldn't be surprised if one can cause real problems with 'random'
destruction orders on non-memory resources, like files. Simple memory
might be safe(r), as releasing order does typically not matter.

Andre'
Elvis Stansvik
2018-10-26 20:26:52 UTC
Permalink
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
Hello,
Post by Elvis Stansvik
I couldn't find a way to contact them.
The best shot would be the std-discussion mailing list, I think.
Post by Elvis Stansvik
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.
FooPrivate constr from vector
Foo constr with arg 0x7fffdb627200
Foo begin 0x7fffdb627200
Foo end 0x7fffdb627200
Foo destr 0x7fffdb627200
FooPrivate destr
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
Which just shows it's working as intended.
However the third test should be a without moveToConst, but storing in a
temporary, i.e. the current best practice. It should output exactly like
the first one, but of course call const begin/end.
For completely other reasons, I came across "Range-based for
statements with initializer" today:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0614r1.html

With that, the Qt best practice could become

for (const auto list = getQList(); const auto &v : list) {
...
}

Which may or may not be pretty, but avoids the extra line of code and
keeps the scope clean.

Elvis
And note the extra move/destruciton in the second example.
Post by Elvis Stansvik
The two unsafe usages are commented out, because they wouldn't compile (good!).
template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}
This won't compile when called on a non-const lvalue (the return type
will be a lvalue reference to const, which won't bind to a non-const
rvalue). I stand corrected.
Which actually makes me think of yet another possibility of misuse, that
is, applying moveToConst to a function returning a const object. That
will compile, using a copy...
Post by Elvis Stansvik
const QVector<Obj> getVector();
for (auto &obj : moveToConst(getVector()) ~~~
Long story short, I think it's good if we stay away from this in Qt.
Clazy warns you if you "do it wrong", and being a Qt-specific problem,
we better not offer too many ways that could have unpleasant drawbacks.
My 2 c,
--
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
Olivier Goffart
2018-10-27 11:37:38 UTC
Permalink
Post by Elvis Stansvik
For completely other reasons, I came across "Range-based for
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0614r1.html
With that, the Qt best practice could become
for (const auto list = getQList(); const auto &v : list) {
...
}
Which may or may not be pretty, but avoids the extra line of code and
keeps the scope clean.
We could even wrap this in a macro for convenience. We would call that macro
'foreach' for example.
Ah no, wait, this name is already taken by a macro that does exactly that :-)

Jokes aside, I think we still should let users use Q_FOREACH for implicitly
shared containers.
--
Olivier

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
Elvis Stansvik
2018-10-27 12:52:48 UTC
Permalink
Post by Olivier Goffart
Post by Elvis Stansvik
For completely other reasons, I came across "Range-based for
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0614r1.html
With that, the Qt best practice could become
for (const auto list = getQList(); const auto &v : list) {
...
}
Which may or may not be pretty, but avoids the extra line of code and
keeps the scope clean.
We could even wrap this in a macro for convenience. We would call that macro
'foreach' for example.
Ah no, wait, this name is already taken by a macro that does exactly that :-)
:)
Post by Olivier Goffart
Jokes aside, I think we still should let users use Q_FOREACH for implicitly
shared containers.
Yes, I thought that Q_FOREACH was slated for deprecation though. But
maybe that's not set in stone yet?

I just found this post by Marc: https://www.kdab.com/goodbye-q_foreach/

Lots of comments there I haven't had time to go through yet, so I bet
it's a contentious issue :)

Elvis
Post by Olivier Goffart
--
Olivier
Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Kevin Kofler
2018-10-27 13:06:06 UTC
Permalink
Post by Elvis Stansvik
Post by Olivier Goffart
Jokes aside, I think we still should let users use Q_FOREACH for
implicitly shared containers.
Yes, I thought that Q_FOREACH was slated for deprecation though. But
maybe that's not set in stone yet?
That's his point, it should be undeprecated.

Kevin Kofler
Elvis Stansvik
2018-10-27 13:39:01 UTC
Permalink
Post by Kevin Kofler
Post by Elvis Stansvik
Post by Olivier Goffart
Jokes aside, I think we still should let users use Q_FOREACH for
implicitly shared containers.
Yes, I thought that Q_FOREACH was slated for deprecation though. But
maybe that's not set in stone yet?
That's his point, it should be undeprecated.
Ah, now I see.

I finally managed to read all the comments on that post, and I
definitely don't want to open that can of worms here again.

I'm fine with Guiseppes answers to my initial question, a qMoveToConst
probably wasn't a good idea in the first place.

Elvis
Post by Kevin Kofler
Kevin Kofler
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Sérgio Martins
2018-10-27 15:27:44 UTC
Permalink
Post by Elvis Stansvik
Post by Olivier Goffart
Jokes aside, I think we still should let users use Q_FOREACH for implicitly
shared containers.
But what's the percentage of Qt developers that understand the
subtleties between Q_FOREACH and range-for ?
Having a toolbox with two similar-but-not-quite constructs is bad for
less seasoned users.
I prefer explicitly assigning the container to a const local variable
instead of relying in the magic behind macros.
Post by Elvis Stansvik
Yes, I thought that Q_FOREACH was slated for deprecation though. But
maybe that's not set in stone yet?
It is, see Qt's documentation:
"Since Qt 5.7, the use of this macro is discouraged. It will be
removed in a future version of Qt"


Regards,
Sérgio
Kevin Kofler
2018-10-28 14:42:45 UTC
Permalink
This post might be inappropriate. Click to display it.
Olivier Goffart
2018-10-29 13:53:20 UTC
Permalink
I'm just replying to this email to sumarize my opinion from the other email in
the "qMoveToConst helper for rvalue references to movable Qt containers?" thread.

I do not think it is time to deprecate foreach. Currently, the documentation
says it is discouraged, and that's fine. But the alternative are harder to use
with Qt containers and I do not think it is wise to tell everybody to port away
from foreach.

One problem of foreach is that it does not work well with QVerLenghtArray and
standard containers. But in practice, Qt user are not using them so much.
But I guess it would be fine to deprecate and warn for this case:
https://codereview.qt-project.org/244010

The other problems seems really minors, and not a reason to port away from it,
especially when the alternative is much more difficult to use right.

On 10/28/18 7:34 PM, Giuseppe D'Angelo via Development wrote:
[...]
Why should the Qt Project invest any resource whatsoever maintaining a solution
to a problem that has also been solved by the C++ language (and in a more
efficient and general way than Q_FOREACH)?
Historical reason, and for the same reason the Qt project maintains its own
containers. I guess it would be good to discourage uses of QVector/QMap/QSet/...
But we can't just force everybody to move away from it just like that.
--
Olivier

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
Olivier Goffart
2018-10-28 18:49:08 UTC
Permalink
Post by Sérgio Martins
Post by Olivier Goffart
Jokes aside, I think we still should let users use Q_FOREACH for implicitly
shared containers.
But what's the percentage of Qt developers that understand the
subtleties between Q_FOREACH and range-for ?
Having a toolbox with two similar-but-not-quite constructs is bad for
less seasoned users.
I prefer explicitly assigning the container to a const local variable
instead of relying in the magic behind macros.
I don't know... what's the percentage of Qt developers that understands when to
use this qAsConst or const copy gymnastic?

It is a bit ironic that one reason given to deprecate Q_FOREACH is that it may
copy the container in some cases, while the alternative has the same problem in
much more common cases. (It is my impression that implicitly shared container
such as QList/QVector are by far much more used than the one that are not
within a typical Qt code base)

What could be done is to only deprecate partial specialization of
qMakeForeachContainer for QVarLenghtArray and the standard containers.
Or for containers that do not have a 'detach' function.
That way, users would get a warning for the problematic containers, but would
continue to work just fine with with the containers most Qt developer use.
--
Olivier

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
Thiago Macieira
2018-10-28 19:17:25 UTC
Permalink
Post by Olivier Goffart
It is a bit ironic that one reason given to deprecate Q_FOREACH is that it
may copy the container in some cases, while the alternative has the same
problem in much more common cases. (It is my impression that implicitly
shared container such as QList/QVector are by far much more used than the
one that are not within a typical Qt code base)
There are two problems with Q_FOREACH:

1) it will copy containers. For Qt containers, that's rather cheap (two atomic
refcount operations), but it's not free. And for Standard Library containers,
that is likely very expensive.

2) it's implemented by way of a for loop inside a for loop, which is known to
throw optimisers out, generating slightly worse code

Our rule already was: Don't use foreach in Qt code. (it was fine in examples)

Using iterators and now using the ranged for may need more typing, but
produces optimal code.
Post by Olivier Goffart
What could be done is to only deprecate partial specialization of
qMakeForeachContainer for QVarLenghtArray and the standard containers.
Or for containers that do not have a 'detach' function.
That way, users would get a warning for the problematic containers, but
would continue to work just fine with with the containers most Qt developer
use.
I disagree. The optimisation problem is not solved.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Olivier Goffart
2018-10-29 11:43:09 UTC
Permalink
Post by Thiago Macieira
Post by Olivier Goffart
It is a bit ironic that one reason given to deprecate Q_FOREACH is that it
may copy the container in some cases, while the alternative has the same
problem in much more common cases. (It is my impression that implicitly
shared container such as QList/QVector are by far much more used than the
one that are not within a typical Qt code base)
1) it will copy containers. For Qt containers, that's rather cheap (two atomic
refcount operations), but it's not free. And for Standard Library containers,
that is likely very expensive.
But using for(:) with a Qt container will cause a detach in the most common
case, so I'd say is even worse.
Which is why i'm saying using this reason is a bit ironic.
Post by Thiago Macieira
2) it's implemented by way of a for loop inside a for loop, which is known to
throw optimisers out, generating slightly worse code
I would consider that the missed optimization is quite small, if not negligible.
And it can be solved in C++17:
https://codereview.qt-project.org/243984/
Post by Thiago Macieira
Our rule already was: Don't use foreach in Qt code. (it was fine in examples)
Using iterators and now using the ranged for may need more typing, but
produces optimal code.
Post by Olivier Goffart
What could be done is to only deprecate partial specialization of
qMakeForeachContainer for QVarLenghtArray and the standard containers.
Or for containers that do not have a 'detach' function.
That way, users would get a warning for the problematic containers, but
would continue to work just fine with with the containers most Qt developer
use.
I disagree. The optimisation problem is not solved.
I'm ok with discouraging Q_FOREACH, but deprecating such a function need more
thinking.
Deprecating means you will force user to port all their codebase away from it,
which is a huge work. If the rationale is just that they will save a couple of
atomic operations, i do not think it is worth it.

Deprecating it only for non-shared container seems more logical, since we then
warn only when there is actually a problem.

And for the Qt shared container case, using foreach is less typing, but also
less error prone. (do i have to use qAsConst? or make a copy? or even return a
const object which even lead to more problems)
--
Olivier

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
André Hartmann
2018-10-29 12:00:07 UTC
Permalink
Hi all,

I fully agree, Olivier.

Looking at https://docs.kdab.com/analysis/qtcreator/clazy.html gives
currently 223 potential detaching containers within range-based for, and
qtbase alone has 46 (https://docs.kdab.com/analysis/qt5/clazy.html).

Even if there may be some false positives, who is going to check and fix
them all? If we don't manage to use the range-based for correctly (for
Qt-containers), why should we force the user to port away from foreach?

Just my two cent.

Best regards,
André
Post by Olivier Goffart
Post by Thiago Macieira
Post by Olivier Goffart
It is a bit ironic that one reason given to deprecate Q_FOREACH is that it
may copy the container in some cases, while the alternative has the same
problem in much more common cases. (It is my impression that implicitly
shared container such as QList/QVector are by far much more used than the
one that are not within a typical Qt code base)
1) it will copy containers. For Qt containers, that's rather cheap (two atomic
refcount operations), but it's not free. And for Standard Library containers,
that is likely very expensive.
But using for(:) with a Qt container will cause a detach in the most
common case, so I'd say is even worse.
Which is why i'm saying using this reason is a bit ironic.
Post by Thiago Macieira
2) it's implemented by way of a for loop inside a for loop, which is known to
throw optimisers out, generating slightly worse code
I would consider that the missed optimization is quite small, if not negligible.
https://codereview.qt-project.org/243984/
Post by Thiago Macieira
Our rule already was: Don't use foreach in Qt code. (it was fine in examples)
Using iterators and now using the ranged for may need more typing, but
produces optimal code.
Post by Olivier Goffart
What could be done is to only deprecate partial specialization of
qMakeForeachContainer for QVarLenghtArray and the standard containers.
Or for containers that do not have a 'detach' function.
That way, users would get a warning for the problematic containers, but
would continue to work just fine with with the containers most Qt developer
use.
I disagree. The optimisation problem is not solved.
I'm ok with discouraging Q_FOREACH, but deprecating such a function need
more thinking.
Deprecating means you will force user to port all their codebase away
from it, which is a huge work. If the rationale is just that they will
save a couple of atomic operations, i do not think it is worth it.
Deprecating it only for non-shared container seems more logical, since
we then warn only when there is actually a problem.
And for the Qt shared container case, using foreach is less typing, but
also less error prone. (do i have to use qAsConst? or make a copy? or
even return a const object which even lead to more problems)
Thiago Macieira
2018-10-29 15:56:12 UTC
Permalink
Post by Olivier Goffart
Post by Thiago Macieira
1) it will copy containers. For Qt containers, that's rather cheap (two
atomic refcount operations), but it's not free. And for Standard Library
containers, that is likely very expensive.
But using for(:) with a Qt container will cause a detach in the most common
case, so I'd say is even worse.
Which is why i'm saying using this reason is a bit ironic.
True, which is why we didn't deprecate, but did add qAsConst.
Post by Olivier Goffart
Post by Thiago Macieira
2) it's implemented by way of a for loop inside a for loop, which is known
to throw optimisers out, generating slightly worse code
I would consider that the missed optimization is quite small, if not
https://codereview.qt-project.org/243984/
I'd solve this the other way around, by making the macro:

if (const auto &_container_ = (container); true) \
for (variable : _container_)

This creates a scope-only const reference to the original container and then
uses the ranged for on it.

We should be able to depend on this for Qt 6.4 or something.
Post by Olivier Goffart
Post by Thiago Macieira
I disagree. The optimisation problem is not solved.
I'm ok with discouraging Q_FOREACH, but deprecating such a function need
more thinking.
I'm with you.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Olivier Goffart
2018-10-29 16:05:37 UTC
Permalink
Post by Thiago Macieira
Post by Olivier Goffart
Post by Thiago Macieira
2) it's implemented by way of a for loop inside a for loop, which is known
to throw optimisers out, generating slightly worse code
I would consider that the missed optimization is quite small, if not
https://codereview.qt-project.org/243984/
if (const auto &_container_ = (container); true) \
for (variable : _container_)
This creates a scope-only const reference to the original container and then
uses the ranged for on it.
That does not work, because foreach need to support a variable which is already
declare: "QString str; foreach (str, list)" (see the documentation
http://doc.qt.io/qt-5/containers.html#foreach ), and this construct is not
allowed in a ranged-based for.

Othewise, we could have that in C++11:

for (variable : qMakeForeachContainer(container))
Post by Thiago Macieira
We should be able to depend on this for Qt 6.4 or something.
I tought Qt6 would probably require C++17. But even if one does not, it's
completely optional.
Philippe
2018-10-31 10:17:43 UTC
Permalink
* if you have a container and you want to do non-mutating iteration, use
for (const auto &c = container; const auto &element : c)
This is enough:

for (auto &element : std::as_const(container))

and for the rvalue reference case:

for (const auto &c = container; auto &element : c)

Philippe

On Wed, 31 Oct 2018 10:47:56 +0100
Deprecating means you will force user to port all their codebase away from it,
which is a huge work. If the rationale is just that they will save a couple of
atomic operations, i do not think it is worth it.
Deprecating it only for non-shared container seems more logical, since we then
warn only when there is actually a problem.
And for the Qt shared container case, using foreach is less typing, but also
less error prone. (do i have to use qAsConst? or make a copy? or even return a
const object which even lead to more problems)
Not really, my main argument is teachability. I want to stop teaching foreach to Qt users (which are also C++ users; why do we keep forgetting that there's a C++ world out there that doesn't use Qt?), and tell them that
* foreach is supposed to be used only on Qt containers, not STL / Boost ones
* actually, only on _certain_ Qt containers,
* but if you use it on a "wrong" container it still works, no warnings or anything, just a tremendous price hit,
* why? because it's unconditionally doing a copy, which is cheap only for the certain containers in Qt,
* and because of this copy it can't do mutating iteration, so you need to learn ranged-based for *anyhow*,
* but thanks to this copy it is perfectly safe to modify the original container, because it's copied anyhow (and you can rely on it) (and documentation was encouraging this) (and Qt's OWN SOURCE CODE had places that relied on that) ---
Well, no. :(
* if you have a container and you want to do mutating iteration, use
for (auto &element : container)
* if you have a container and you want to do non-mutating iteration, use
for (const auto &c = container; const auto &element : c)
And that's it; this is a pure C++ solution that works for any container (Qt, STL, Boost, ...), does never take any copy (you're iterating on the original), it is const-correct (can't accidentally modify the container through the element in a non-mutating iteration, can't accidentally modify a const container), never detaches a Qt container unless you need to anyhow (mutating iteration), and in general it is not safe to modify the container's structure from within the loop's body so don't do it.
My 2 c,
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
Thiago Macieira
2018-10-31 15:30:28 UTC
Permalink
On Wednesday, 31 October 2018 03:35:32 PDT Giuseppe D'Angelo via Development
Post by Philippe
for (const auto &c = container; auto &element : c)
Why not using this one for *both* lvalues and rvalues? Once more, one
fewer thing to teach.
C++17 required (and I didn't know this specific syntax worked).
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Philippe
2018-10-31 15:52:10 UTC
Permalink
Post by Thiago Macieira
C++17 required (and I didn't know this specific syntax worked).
AFAIR, this is in fact a C++20 proposal.

Hence best is:

for (auto &element : std::as_const(container))

Philippe

On Wed, 31 Oct 2018 08:30:28 -0700
Post by Thiago Macieira
On Wednesday, 31 October 2018 03:35:32 PDT Giuseppe D'Angelo via Development
Post by Philippe
for (const auto &c = container; auto &element : c)
Why not using this one for *both* lvalues and rvalues? Once more, one
fewer thing to teach.
C++17 required (and I didn't know this specific syntax worked).
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Thiago Macieira
2018-10-31 16:47:14 UTC
Permalink
Post by Philippe
for (auto &element : std::as_const(container))
Which is why we added qAsConst.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Sérgio Martins
2018-10-27 15:33:30 UTC
Permalink
Post by Elvis Stansvik
Hi all (first post),
Welcome :)
Post by Elvis Stansvik
In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.
For good reasons there's no version of qAsConst that takes an rvalue
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
Not sure what's the reason we don't do it in Qt. It's frowned upon for
regular value types, but our containers are special.

Regards,
Sergio
Elvis Stansvik
2018-10-27 15:46:00 UTC
Permalink
Post by Sérgio Martins
Post by Elvis Stansvik
Hi all (first post),
Welcome :)
Post by Elvis Stansvik
In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.
For good reasons there's no version of qAsConst that takes an rvalue
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
Not sure what's the reason we don't do it in Qt. It's frowned upon for
regular value types, but our containers are special.
It was the very last comment on that blog post, to which Marc
answered: "Making returned containers const inhibits move semantics,
because const rvalues do not bind to the mutable rvalue references
that move constructors and move assignment operators use."

Elvis
Post by Sérgio Martins
Regards,
Sergio
André Pönitz
2018-10-27 17:29:31 UTC
Permalink
Post by Sérgio Martins
Post by Elvis Stansvik
Hi all (first post),
Welcome :)
Post by Elvis Stansvik
In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.
For good reasons there's no version of qAsConst that takes an rvalue
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
This is actually a route we recently took in some cases in Qt Creator's
code base.

Andre'
Lars Knoll
2018-10-27 17:48:45 UTC
Permalink
On 27 Oct 2018, at 19:29, André Pönitz <***@t-online.de<mailto:***@t-online.de>> wrote:

On Sat, Oct 27, 2018 at 04:33:30PM +0100, Sérgio Martins wrote:
On Sat, Oct 20, 2018 at 1:44 PM Elvis Stansvik <***@gmail.com<mailto:***@gmail.com>> wrote:

Hi all (first post),

Welcome :)

In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.

For good reasons there's no version of qAsConst that takes an rvalue
reference, so you can't do e.g. for (auto foo :
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.

Should we instead just encourage people to make returnsQtContainer()
return a const container ?

This is actually a route we recently took in some cases in Qt Creator's
code base.

That might actually make sense. Calling a non const method on the returned temporary object is usually a mistake anyway. And the copy/move assignment to a variable will work with the const object.

Cheers,
Lars
Elvis Stansvik
2018-10-27 19:07:33 UTC
Permalink
Post by Elvis Stansvik
Hi all (first post),
Welcome :)
In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.
For good reasons there's no version of qAsConst that takes an rvalue
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
This is actually a route we recently took in some cases in Qt Creator's
code base.
That might actually make sense. Calling a non const method on the returned temporary object is usually a mistake anyway. And the copy/move assignment to a variable will work with the const object.
Hm, but was Marc not right when he said

"Making returned containers const inhibits move semantics, because
const rvalues do not bind to the mutable rvalue references that move
constructors and move assignment operators use."

on his blog?

Guess he should jump in here to defend himself :)

Elvis
Post by Elvis Stansvik
Cheers,
Lars
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-27 19:37:06 UTC
Permalink
Post by Elvis Stansvik
Post by Elvis Stansvik
Hi all (first post),
Welcome :)
In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.
For good reasons there's no version of qAsConst that takes an rvalue
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
This is actually a route we recently took in some cases in Qt Creator's
code base.
That might actually make sense. Calling a non const method on the returned temporary object is usually a mistake anyway. And the copy/move assignment to a variable will work with the const object.
Hm, but was Marc not right when he said
"Making returned containers const inhibits move semantics, because
const rvalues do not bind to the mutable rvalue references that move
constructors and move assignment operators use."
on his blog?
Guess he should jump in here to defend himself :)
Another problem I found when trying to apply this to our code base is
that it seems you can't QtConcurrent::run a function returning const
T, because internally, ResultStoreBase::addResult looks like

template <typename T>
int addResult(int index, const T *result)
{
if (result == 0)
return addResult(index, static_cast<void *>(nullptr));
else
return addResult(index, static_cast<void *>(new T(*result)));
}

so you'll get an error like

/usr/include/x86_64-linux-gnu/qt5/QtCore/qresultstore.h:151: error:
static_cast from 'const QVector<double> *' to 'void *' is not allowed
return addResult(index, static_cast<void *>(new T(*result)));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Just a small downside in our case, but still.

Elvis
Post by Elvis Stansvik
Elvis
Post by Elvis Stansvik
Cheers,
Lars
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Lars Knoll
2018-10-27 20:07:40 UTC
Permalink
On 27 Oct 2018, at 21:07, Elvis Stansvik <***@gmail.com<mailto:***@gmail.com>> wrote:

Den lör 27 okt. 2018 kl 19:48 skrev Lars Knoll <***@qt.io<mailto:***@qt.io>>:



On 27 Oct 2018, at 19:29, André Pönitz <***@t-online.de<mailto:***@t-online.de>> wrote:

On Sat, Oct 27, 2018 at 04:33:30PM +0100, Sérgio Martins wrote:

On Sat, Oct 20, 2018 at 1:44 PM Elvis Stansvik <***@gmail.com<mailto:***@gmail.com>> wrote:


Hi all (first post),


Welcome :)

In Qt 5.7+ there's qAsConst, an std::as_const implementation for those
who are not on C++17 yet, which is convenient for iterating over Qt
containers using range-based for loops without causing a detach.

For good reasons there's no version of qAsConst that takes an rvalue
reference, so you can't do e.g. for (auto foo :
qAsConst(returnsQtContainer()) { ... }. Instead you must do const
auto stuff = returnsQtContainer(); for (auto foo : stuff) { ... }.


Should we instead just encourage people to make returnsQtContainer()
return a const container ?


This is actually a route we recently took in some cases in Qt Creator's
code base.


That might actually make sense. Calling a non const method on the returned temporary object is usually a mistake anyway. And the copy/move assignment to a variable will work with the const object.

Hm, but was Marc not right when he said

"Making returned containers const inhibits move semantics, because
const rvalues do not bind to the mutable rvalue references that move
constructors and move assignment operators use."

on his blog?

Guess he should jump in here to defend himself :)

No need. He’s right. A move constructor only works with a non const value, as it needs to modify the object. One thing to check however for our containers is how much more expensive the copy is (compared to the move).

Cheers,
Lars


Elvis


Cheers,
Lars

_______________________________________________
Development mailing list
***@qt-project.org<mailto:***@qt-project.org>
http://lists.qt-project.org/mailman/listinfo/development
Thiago Macieira
2018-10-27 20:35:05 UTC
Permalink
No need. He’s right. A move constructor only works with a non const value,
as it needs to modify the object. One thing to check however for our
containers is how much more expensive the copy is (compared to the move).
Two atomic operations.

https://stackoverflow.com/questions/2538070/atomic-operation-cost
https://stackoverflow.com/questions/34660376/atomic-fetch-add-vs-add-performance
etc.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Thiago Macieira
2018-10-27 20:23:00 UTC
Permalink
Post by Sérgio Martins
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
We should not, since the prevents move-construction from happening. You'll pay
the cost of two reference counts (one up and one down).
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Elvis Stansvik
2018-10-27 21:14:51 UTC
Permalink
Post by Thiago Macieira
Post by Sérgio Martins
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
We should not, since the prevents move-construction from happening. You'll pay
the cost of two reference counts (one up and one down).
Alright, rules that out I guess. From the looks of it, it could be ~100 ns.

Elvis
Post by Thiago Macieira
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-28 10:22:10 UTC
Permalink
Post by Thiago Macieira
Post by Sérgio Martins
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
We should not, since the prevents move-construction from happening. You'll pay
the cost of two reference counts (one up and one down).
Though hmm, even if we'd lose move-construction, for the copy we'd get
instead, wouldn't copy elision kick in and elide it? So we wouldn't
have to pay for the ref count up/down?

I simplified my example a little, so to recap:

$ cat copytest.cpp
#include <iostream>
#include <vector>

struct Foo {
Foo() {
std::cout << this << " constructed" << std::endl;
}
Foo(const Foo &) {
std::cout << this << " copy constructed" << std::endl;
}
Foo(Foo &&) {
std::cout << this << " move constructed" << std::endl;
}
~Foo() {
std::cout << this << " destructed" << std::endl;
}
std::vector<int>::iterator begin() {
std::cout << this << " begin" << std::endl; return v.begin();
};
std::vector<int>::const_iterator begin() const {
std::cout << this << " begin const" << std::endl; return v.begin();
};
std::vector<int>::iterator end() {
std::cout << this << " end" << std::endl; return v.end();
};
std::vector<int>::const_iterator end() const {
std::cout << this << " end const" << std::endl; return v.end();
};
std::vector<int> v{1, 2, 3};
};

Foo f() {
Foo foo;
return foo;
}

const Foo constF() {
Foo foo;
return foo;
}

template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}

int main(void) {
std::cout << "without moveToConst:" << std::endl;
for (auto v : f()) { }

std::cout << std::endl;

std::cout << "with moveToConst:" << std::endl;
for (auto v : moveToConst(f())) { }

std::cout << std::endl;

std::cout << "with returning const:" << std::endl;
for (auto v : constF()) { }

std::cout << std::endl;

std::cout << "with recommended way:" << std::endl;
const auto stuff = f();
for (auto v : stuff) { }

return 0;
}

Result:

$ g++ -std=c++17 -O0 -o copytest copytest.cpp
$ ./copytest
without moveToConst:
0x7ffcc6ac76b0 constructed
0x7ffcc6ac76b0 begin
0x7ffcc6ac76b0 end
0x7ffcc6ac76b0 destructed

with moveToConst:
0x7ffcc6ac7710 constructed
0x7ffcc6ac76d0 move constructed
0x7ffcc6ac7710 destructed
0x7ffcc6ac76d0 begin const
0x7ffcc6ac76d0 end const
0x7ffcc6ac76d0 destructed

with returning const:
0x7ffcc6ac76f0 constructed
0x7ffcc6ac76f0 begin const
0x7ffcc6ac76f0 end const
0x7ffcc6ac76f0 destructed

with recommended way:
0x7ffcc6ac7710 constructed
0x7ffcc6ac7710 begin const
0x7ffcc6ac7710 end const
0x7ffcc6ac7710 destructed

So it looks like the copy has been elided also in the "with returning
const" case. Anyone know if this is guaranteed in C++17, or just
permitted?

If I compile with -fno-elide-constructors to disable elision:

$ g++ -std=c++17 -fno-elide-constructors -O0 -o copytest copytest.cpp
$ ./copytest
without moveToConst:
0x7ffe7c107070 constructed
0x7ffe7c1070f0 move constructed
0x7ffe7c107070 destructed
0x7ffe7c1070f0 begin
0x7ffe7c1070f0 end
0x7ffe7c1070f0 destructed

with moveToConst:
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107110 move constructed
0x7ffe7c107150 destructed
0x7ffe7c107110 begin const
0x7ffe7c107110 end const
0x7ffe7c107110 destructed

with returning const:
0x7ffe7c107070 constructed
0x7ffe7c107130 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107130 begin const
0x7ffe7c107130 end const
0x7ffe7c107130 destructed

with recommended way:
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107150 begin const
0x7ffe7c107150 end const
0x7ffe7c107150 destructed
$

What I find surprising here is that with the -fno-elide-constructors
flag, in the "with returning const", the move constructor *is* called.

Didn't we just say it wouldn't, because the const rvalue wouldn't bind
to the non-const rvalue reference?

I'm a little confused :p

Elvis
Post by Thiago Macieira
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-28 10:28:17 UTC
Permalink
Post by Elvis Stansvik
Post by Thiago Macieira
Post by Sérgio Martins
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
We should not, since the prevents move-construction from happening. You'll pay
the cost of two reference counts (one up and one down).
Though hmm, even if we'd lose move-construction, for the copy we'd get
instead, wouldn't copy elision kick in and elide it? So we wouldn't
have to pay for the ref count up/down?
$ cat copytest.cpp
#include <iostream>
#include <vector>
struct Foo {
Foo() {
std::cout << this << " constructed" << std::endl;
}
Foo(const Foo &) {
std::cout << this << " copy constructed" << std::endl;
}
Foo(Foo &&) {
std::cout << this << " move constructed" << std::endl;
}
~Foo() {
std::cout << this << " destructed" << std::endl;
}
std::vector<int>::iterator begin() {
std::cout << this << " begin" << std::endl; return v.begin();
};
std::vector<int>::const_iterator begin() const {
std::cout << this << " begin const" << std::endl; return v.begin();
};
std::vector<int>::iterator end() {
std::cout << this << " end" << std::endl; return v.end();
};
std::vector<int>::const_iterator end() const {
std::cout << this << " end const" << std::endl; return v.end();
};
std::vector<int> v{1, 2, 3};
};
Foo f() {
Foo foo;
return foo;
}
const Foo constF() {
Foo foo;
return foo;
}
template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}
int main(void) {
std::cout << "without moveToConst:" << std::endl;
for (auto v : f()) { }
std::cout << std::endl;
std::cout << "with moveToConst:" << std::endl;
for (auto v : moveToConst(f())) { }
std::cout << std::endl;
std::cout << "with returning const:" << std::endl;
for (auto v : constF()) { }
std::cout << std::endl;
std::cout << "with recommended way:" << std::endl;
const auto stuff = f();
for (auto v : stuff) { }
return 0;
}
I put it up on godbolt for this who want to play: https://godbolt.org/z/x6FPq_

Elvis
Post by Elvis Stansvik
$ g++ -std=c++17 -O0 -o copytest copytest.cpp
$ ./copytest
0x7ffcc6ac76b0 constructed
0x7ffcc6ac76b0 begin
0x7ffcc6ac76b0 end
0x7ffcc6ac76b0 destructed
0x7ffcc6ac7710 constructed
0x7ffcc6ac76d0 move constructed
0x7ffcc6ac7710 destructed
0x7ffcc6ac76d0 begin const
0x7ffcc6ac76d0 end const
0x7ffcc6ac76d0 destructed
0x7ffcc6ac76f0 constructed
0x7ffcc6ac76f0 begin const
0x7ffcc6ac76f0 end const
0x7ffcc6ac76f0 destructed
0x7ffcc6ac7710 constructed
0x7ffcc6ac7710 begin const
0x7ffcc6ac7710 end const
0x7ffcc6ac7710 destructed
So it looks like the copy has been elided also in the "with returning
const" case. Anyone know if this is guaranteed in C++17, or just
permitted?
$ g++ -std=c++17 -fno-elide-constructors -O0 -o copytest copytest.cpp
$ ./copytest
0x7ffe7c107070 constructed
0x7ffe7c1070f0 move constructed
0x7ffe7c107070 destructed
0x7ffe7c1070f0 begin
0x7ffe7c1070f0 end
0x7ffe7c1070f0 destructed
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107110 move constructed
0x7ffe7c107150 destructed
0x7ffe7c107110 begin const
0x7ffe7c107110 end const
0x7ffe7c107110 destructed
0x7ffe7c107070 constructed
0x7ffe7c107130 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107130 begin const
0x7ffe7c107130 end const
0x7ffe7c107130 destructed
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107150 begin const
0x7ffe7c107150 end const
0x7ffe7c107150 destructed
$
What I find surprising here is that with the -fno-elide-constructors
flag, in the "with returning const", the move constructor *is* called.
Didn't we just say it wouldn't, because the const rvalue wouldn't bind
to the non-const rvalue reference?
I'm a little confused :p
Elvis
Post by Thiago Macieira
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-28 11:13:50 UTC
Permalink
Post by Elvis Stansvik
Post by Thiago Macieira
Post by Sérgio Martins
Should we instead just encourage people to make returnsQtContainer()
return a const container ?
We should not, since the prevents move-construction from happening. You'll pay
the cost of two reference counts (one up and one down).
Though hmm, even if we'd lose move-construction, for the copy we'd get
instead, wouldn't copy elision kick in and elide it? So we wouldn't
have to pay for the ref count up/down?
$ cat copytest.cpp
#include <iostream>
#include <vector>
struct Foo {
Foo() {
std::cout << this << " constructed" << std::endl;
}
Foo(const Foo &) {
std::cout << this << " copy constructed" << std::endl;
}
Foo(Foo &&) {
std::cout << this << " move constructed" << std::endl;
}
~Foo() {
std::cout << this << " destructed" << std::endl;
}
std::vector<int>::iterator begin() {
std::cout << this << " begin" << std::endl; return v.begin();
};
std::vector<int>::const_iterator begin() const {
std::cout << this << " begin const" << std::endl; return v.begin();
};
std::vector<int>::iterator end() {
std::cout << this << " end" << std::endl; return v.end();
};
std::vector<int>::const_iterator end() const {
std::cout << this << " end const" << std::endl; return v.end();
};
std::vector<int> v{1, 2, 3};
};
Foo f() {
Foo foo;
return foo;
}
const Foo constF() {
Foo foo;
return foo;
}
template <typename T>
const T moveToConst(T &&t)
{
return std::move(t);
}
int main(void) {
std::cout << "without moveToConst:" << std::endl;
for (auto v : f()) { }
std::cout << std::endl;
std::cout << "with moveToConst:" << std::endl;
for (auto v : moveToConst(f())) { }
std::cout << std::endl;
std::cout << "with returning const:" << std::endl;
for (auto v : constF()) { }
std::cout << std::endl;
std::cout << "with recommended way:" << std::endl;
const auto stuff = f();
for (auto v : stuff) { }
return 0;
}
$ g++ -std=c++17 -O0 -o copytest copytest.cpp
$ ./copytest
0x7ffcc6ac76b0 constructed
0x7ffcc6ac76b0 begin
0x7ffcc6ac76b0 end
0x7ffcc6ac76b0 destructed
0x7ffcc6ac7710 constructed
0x7ffcc6ac76d0 move constructed
0x7ffcc6ac7710 destructed
0x7ffcc6ac76d0 begin const
0x7ffcc6ac76d0 end const
0x7ffcc6ac76d0 destructed
0x7ffcc6ac76f0 constructed
0x7ffcc6ac76f0 begin const
0x7ffcc6ac76f0 end const
0x7ffcc6ac76f0 destructed
0x7ffcc6ac7710 constructed
0x7ffcc6ac7710 begin const
0x7ffcc6ac7710 end const
0x7ffcc6ac7710 destructed
So it looks like the copy has been elided also in the "with returning
const" case. Anyone know if this is guaranteed in C++17, or just
permitted?
$ g++ -std=c++17 -fno-elide-constructors -O0 -o copytest copytest.cpp
$ ./copytest
0x7ffe7c107070 constructed
0x7ffe7c1070f0 move constructed
0x7ffe7c107070 destructed
0x7ffe7c1070f0 begin
0x7ffe7c1070f0 end
0x7ffe7c1070f0 destructed
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107110 move constructed
0x7ffe7c107150 destructed
0x7ffe7c107110 begin const
0x7ffe7c107110 end const
0x7ffe7c107110 destructed
0x7ffe7c107070 constructed
0x7ffe7c107130 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107130 begin const
0x7ffe7c107130 end const
0x7ffe7c107130 destructed
0x7ffe7c107070 constructed
0x7ffe7c107150 move constructed
0x7ffe7c107070 destructed
0x7ffe7c107150 begin const
0x7ffe7c107150 end const
0x7ffe7c107150 destructed
$
I tried some more compilers.

With Apple Clang 9.0.0 and -std=c++14, the output is the same without
-fno-elide-constructors, but interestingly, if -fno-elide-constructors
is added, the output is:

with returning const:
0x7fff596ed8a8 constructed
0x7fff596eda20 move constructed
0x7fff596ed8a8 destructed
0x7fff596eda20 begin const
0x7fff596eda20 end const
0x7fff596eda20 destructed

with recommended way:
0x7fff596ed8a8 constructed
0x7fff596ed9c8 move constructed
0x7fff596ed8a8 destructed
0x7fff596ed9e0 move constructed
0x7fff596ed9c8 destructed
0x7fff596ed9e0 begin const
0x7fff596ed9e0 end const
0x7fff596ed9e0 destructed

With MSVC 2015 (compiled with just cl copytest.cpp), the output is:

with returning const:
0000009FFE4BFD90 constructed
0000009FFE4BFE88 move constructed
0000009FFE4BFD90 destructed
0000009FFE4BFE88 begin const
0000009FFE4BFE88 end const
0000009FFE4BFE88 destructed

with recommended way:
0000009FFE4BFD90 constructed
0000009FFE4BFEA0 move constructed
0000009FFE4BFD90 destructed
0000009FFE4BFEA0 begin const
0000009FFE4BFEA0 end const
0000009FFE4BFEA0 destructed

So somehow it invokes the move constructor, not sure if this is just a bug.

Elvis
Post by Elvis Stansvik
What I find surprising here is that with the -fno-elide-constructors
flag, in the "with returning const", the move constructor *is* called.
Didn't we just say it wouldn't, because the const rvalue wouldn't bind
to the non-const rvalue reference?
I'm a little confused :p
Elvis
Post by Thiago Macieira
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
_______________________________________________
Development mailing list
http://lists.qt-project.org/mailman/listinfo/development
Elvis Stansvik
2018-10-28 12:42:10 UTC
Permalink
Den sön 28 okt. 2018 kl 13:32 skrev Giuseppe D'Angelo via Development
Post by Elvis Stansvik
Though hmm, even if we'd lose move-construction, for the copy we'd get
instead, wouldn't copy elision kick in and elide it? So we wouldn't
have to pay for the ref count up/down?
GCE is one thing, and applies in a very specific case (returning a
prvalue).
(N)RVO is another thing, and may or may not be applied depending on
whether the compiler *can* apply it and *will* apply it.
In the general case, you won't have either, and thus having a move will
be cheaper than a copy. Returning const objects for types which benefit
from moves is a bad idea.
Ah yes of course, my bad.

Elvis
My 2 c,
--
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
http://lists.qt-project.org/mailman/listinfo/development
Loading...