Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Wednesday, 31 October 2018 08:52:10 PDT Philippe wrote: > Hence best is: > > for (auto : std::as_const(container)) Which is why we added qAsConst. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 31/10/18 16:52, Philippe ha scritto: AFAIR, this is in fact a C++20 proposal. Yes, it is (only if / switch got the optional initializer in C++17). Anyhow, nothing special about this: 1) can just do the same in pre-C++2a in two lines 2) not using C++-latest costs more :-) Cheers, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
> 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 : std::as_const(container)) Philippe On Wed, 31 Oct 2018 08:30:28 -0700 Thiago Macieira wrote: > On Wednesday, 31 October 2018 03:35:32 PDT Giuseppe D'Angelo via Development > wrote: > > > for (const auto = container; auto : 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 > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Wednesday, 31 October 2018 03:35:32 PDT Giuseppe D'Angelo via Development wrote: > > for (const auto = container; auto : 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 Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 31/10/18 11:17, Philippe ha scritto: and for the rvalue reference case: for (const auto = container; auto : c) Why not using this one for *both* lvalues and rvalues? Once more, one fewer thing to teach. Cheers, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Good post, but: >> * if you have a container and you want to do non-mutating iteration, use >> for (const auto = container; const auto : c) This is enough: for (auto : std::as_const(container)) and for the rvalue reference case: for (const auto = container; auto : c) Philippe On Wed, 31 Oct 2018 10:47:56 +0100 Giuseppe D'Angelo via Development wrote: > Il 29/10/18 12:43, Olivier Goffart ha scritto: > > 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. :( > > > I want to teach users: > > * if you have a container and you want to do mutating iteration, use > > for (auto : container) > > > * if you have a container and you want to do non-mutating iteration, use > > for (const auto = container; const auto : 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, > > -- Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 29/10/18 12:43, Olivier Goffart ha scritto: 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. :( I want to teach users: * if you have a container and you want to do mutating iteration, use for (auto : container) * if you have a container and you want to do non-mutating iteration, use for (const auto = container; const auto : 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, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 29/10/18 13:00, André Hartmann ha scritto: Looking athttps://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? Well, noone is forcing you to change those usages right now; updating old practices is something which is naturally part of maintaining a code base. And, yes, *we* get this stuff wrong all the time (... QList anyone?). Luckily we now have something like clazy now, so we can prevent such mistakes from happening in the first place. My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 2018-10-29 15:56, Thiago Macieira wrote: On Monday, 29 October 2018 04:43:09 PDT Olivier Goffart wrote: > 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. > 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/ I'd solve this the other way around, by making the macro: if (const auto &_container_ = (container); true) \ for (variable : _container_) That's not behaviour-compatible and will introduce bugs where users rely on iterating over a copy. Regards, -- Sérgio Martins | sergio.mart...@kdab.com | Senior Software Engineer Klarälvdalens Datakonsult AB, a KDAB Group company Tel: Sweden (HQ) +46-563-540090, USA +1-866-777-KDAB(5322) KDAB - The Qt, C++ and OpenGL Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 10/29/18 4:56 PM, Thiago Macieira wrote: 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/ 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. 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)) 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. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Monday, 29 October 2018 04:43:09 PDT Olivier Goffart wrote: > > 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. > > 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/ 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. > > 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
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é Am 29.10.18 um 12:43 schrieb Olivier Goffart: On 10/28/18 8:17 PM, Thiago Macieira wrote: On Sunday, 28 October 2018 11:49:08 PDT Olivier Goffart wrote: 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. 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. 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/ 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. 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) ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 10/28/18 8:17 PM, Thiago Macieira wrote: On Sunday, 28 October 2018 11:49:08 PDT Olivier Goffart wrote: 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. 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. 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/ 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. 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Sunday, 28 October 2018 11:49:08 PDT Olivier Goffart wrote: > 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. > 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 10/27/18 5:27 PM, Sérgio Martins wrote: Den lör 27 okt. 2018 kl 13:37 skrev 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Sérgio Martins wrote: > On Sat, Oct 27, 2018 at 1:53 PM Elvis Stansvik wrote: >> 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" As long as it is not actually removed, it is not too late to undo this totally incorrect decision. See also: https://valdyas.org/fading/hacking/happy-porting/ (Technically, it could be readded even after it is removed, but it would be much easier to undeprecate it now.) Kevin Kofler ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 28 okt. 2018 kl 13:32 skrev Giuseppe D'Angelo via Development : > > Il 28/10/18 11:22, Elvis Stansvik ha scritto: > > 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, > > -- > Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 27/10/18 17:27, Sérgio Martins ha scritto: 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" ... we need to start adding actual deprecation macros, or people will never notice. https://codereview.qt-project.org/#/c/243949/ Cheers, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 28/10/18 11:22, Elvis Stansvik ha scritto: 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. My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 28 okt. 2018 kl 11:22 skrev Elvis Stansvik :
>
> Den lör 27 okt. 2018 kl 22:23 skrev Thiago Macieira
> :
> >
> > On Saturday, 27 October 2018 08:33:30 PDT Sérgio Martins wrote:
> > > 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
> #include
>
> 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::iterator begin() {
> std::cout << this << " begin" << std::endl; return v.begin();
> };
> std::vector::const_iterator begin() const {
> std::cout << this << " begin const" << std::endl; return v.begin();
> };
> std::vector::iterator end() {
> std::cout << this << " end" << std::endl; return v.end();
> };
> std::vector::const_iterator end() const {
> std::cout << this << " end const" << std::endl; return v.end();
> };
> std::vector v{1, 2, 3};
> };
>
> Foo f() {
> Foo foo;
> return foo;
> }
>
> const Foo constF() {
> Foo foo;
> return foo;
> }
>
> template
> const T moveToConst(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
> $
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:
009FFE4BFD90 constructed
009FFE4BFE88 move constructed
009FFE4BFD90
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 28 okt. 2018 kl 11:22 skrev Elvis Stansvik :
>
> Den lör 27 okt. 2018 kl 22:23 skrev Thiago Macieira
> :
> >
> > On Saturday, 27 October 2018 08:33:30 PDT Sérgio Martins wrote:
> > > 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
> #include
>
> 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::iterator begin() {
> std::cout << this << " begin" << std::endl; return v.begin();
> };
> std::vector::const_iterator begin() const {
> std::cout << this << " begin const" << std::endl; return v.begin();
> };
> std::vector::iterator end() {
> std::cout << this << " end" << std::endl; return v.end();
> };
> std::vector::const_iterator end() const {
> std::cout << this << " end const" << std::endl; return v.end();
> };
> std::vector v{1, 2, 3};
> };
>
> Foo f() {
> Foo foo;
> return foo;
> }
>
> const Foo constF() {
> Foo foo;
> return foo;
> }
>
> template
> const T moveToConst(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
>
> 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
>
> >
> > --
> > Thiago Macieira - thiago.macieira (AT) intel.com
> > Software Architect - Intel Open Source Technology Center
> >
> >
> >
> > ___
> > Development mailing list
> > Development@qt-project.org
> > http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 22:23 skrev Thiago Macieira :
>
> On Saturday, 27 October 2018 08:33:30 PDT Sérgio Martins wrote:
> > 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
#include
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::iterator begin() {
std::cout << this << " begin" << std::endl; return v.begin();
};
std::vector::const_iterator begin() const {
std::cout << this << " begin const" << std::endl; return v.begin();
};
std::vector::iterator end() {
std::cout << this << " end" << std::endl; return v.end();
};
std::vector::const_iterator end() const {
std::cout << this << " end const" << std::endl; return v.end();
};
std::vector v{1, 2, 3};
};
Foo f() {
Foo foo;
return foo;
}
const Foo constF() {
Foo foo;
return foo;
}
template
const T moveToConst(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
>
> --
> Thiago Macieira - thiago.macieira (AT) intel.com
> Software Architect - Intel Open Source Technology Center
>
>
>
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 22:23 skrev Thiago Macieira : > > On Saturday, 27 October 2018 08:33:30 PDT Sérgio Martins wrote: > > 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 > > -- > Thiago Macieira - thiago.macieira (AT) intel.com > Software Architect - Intel Open Source Technology Center > > > > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Saturday, 27 October 2018 13:07:40 PDT Lars Knoll wrote: > 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Saturday, 27 October 2018 08:33:30 PDT Sérgio Martins wrote: > 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 27 Oct 2018, at 21:07, Elvis Stansvik
mailto:elvst...@gmail.com>> wrote:
Den lör 27 okt. 2018 kl 19:48 skrev Lars Knoll
mailto:lars.kn...@qt.io>>:
On 27 Oct 2018, at 19:29, André Pönitz
mailto:apoen...@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
mailto:elvst...@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
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 21:07 skrev Elvis Stansvik :
>
> Den lör 27 okt. 2018 kl 19:48 skrev Lars Knoll :
> >
> >
> >
> > On 27 Oct 2018, at 19:29, André Pönitz 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 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 :)
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
int addResult(int index, const T *result)
{
if (result == 0)
return addResult(index, static_cast(nullptr));
else
return addResult(index, static_cast(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 *' to 'void *' is not allowed
return addResult(index, static_cast(new T(*result)));
^~~
Just a small downside in our case, but still.
Elvis
>
> Elvis
>
> >
> > Cheers,
> > Lars
> >
> > ___
> > Development mailing list
> > Development@qt-project.org
> > http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 19:48 skrev Lars Knoll :
>
>
>
> On 27 Oct 2018, at 19:29, André Pönitz 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 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 :)
Elvis
>
> Cheers,
> Lars
>
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 27 Oct 2018, at 19:29, André Pönitz
mailto:apoen...@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
mailto:elvst...@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
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
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 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.
Andre'
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 17:33 skrev Sérgio Martins :
>
> On Sat, Oct 20, 2018 at 1:44 PM Elvis Stansvik 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 ?
> 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
>
> Regards,
> Sergio
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Sat, Oct 20, 2018 at 1:44 PM Elvis Stansvik 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 ?
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
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
> Den lör 27 okt. 2018 kl 13:37 skrev 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. On Sat, Oct 27, 2018 at 1:53 PM Elvis Stansvik wrote: > 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 15:06 skrev Kevin Kofler : > > Elvis Stansvik wrote: > > Den lör 27 okt. 2018 kl 13:37 skrev 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 > > Kevin Kofler > > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Elvis Stansvik wrote: > Den lör 27 okt. 2018 kl 13:37 skrev 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 27 okt. 2018 kl 13:37 skrev Olivier Goffart :
>
> On 10/26/18 10:26 PM, Elvis Stansvik wrote:
> > 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 : 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.
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
>
> --
> Olivier
>
> Woboq - Qt services and support - https://woboq.com - https://code.woboq.org
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On 10/26/18 10:26 PM, Elvis Stansvik wrote:
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 : 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
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
:
>
> Hello,
>
> Il 21/10/18 16:15, Elvis Stansvik ha scritto:
> > I couldn't find a way to contact them.
>
> The best shot would be the std-discussion mailing list, I think.
>
> > 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@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@newton move-to-const-test]$
> >
> > 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 : 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.
>
>
> > The two unsafe usages are commented out, because they wouldn't compile
> > (good!).
>
>
> Whops, you're absolutely right. My bad. With your proposed implementation:
>
> > template
> > const T moveToConst(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...
>
> > const QVector getVector();
> > for (auto : 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,
> --
> Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Hi, Il 22/10/18 21:40, André Pönitz ha scritto: 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. What bad behaviour are we referring to here? 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. Oh, it absolutely does. That's why in Qt we implement move assignment in terms of pure swap if and only if the only resource that a container holds is memory (e.g. QString, QByteArray). For all other cases (e.g. QVector) we implement move assignment as move-and-swap, to be sure that resources are deterministically destroyed when the move happens. My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
On Sun, Oct 21, 2018 at 04:15:58PM +0200, Elvis Stansvik wrote: > 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@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@newton move-to-const-test]$ > > 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' ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
:
>
> Hello,
>
> Il 21/10/18 16:15, Elvis Stansvik ha scritto:
> > I couldn't find a way to contact them.
>
> The best shot would be the std-discussion mailing list, I think.
>
> > 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@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@newton move-to-const-test]$
> >
> > 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.
>
>
> > The two unsafe usages are commented out, because they wouldn't compile
> > (good!).
>
>
> Whops, you're absolutely right. My bad. With your proposed implementation:
>
> > template
> > const T moveToConst(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...
>
> > const QVector getVector();
> > for (auto : 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,
> --
> Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 21 okt. 2018 kl 17:50 skrev Giuseppe D'Angelo
:
>
> Hello,
>
> Il 21/10/18 16:15, Elvis Stansvik ha scritto:
> > I couldn't find a way to contact them.
>
> The best shot would be the std-discussion mailing list, I think.
>
> > 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@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@newton move-to-const-test]$
> >
> > 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.
>
>
> > The two unsafe usages are commented out, because they wouldn't compile
> > (good!).
>
>
> Whops, you're absolutely right. My bad. With your proposed implementation:
>
> > template
> > const T moveToConst(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...
>
> > const QVector getVector();
> > for (auto : 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,
> --
> Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 21/10/18 17:50, Giuseppe D'Angelo via Development ha scritto: This won't compile when called on a non-const lvalue (the return type will be a lvalue reference to const, To NON-const, that is. -- Giuseppe D'Angelo | giuseppe.dang...@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 smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Hello,
Il 21/10/18 16:15, Elvis Stansvik ha scritto:
I couldn't find a way to contact them.
The best shot would be the std-discussion mailing list, I think.
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@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@newton move-to-const-test]$
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.
The two unsafe usages are commented out, because they wouldn't compile (good!).
Whops, you're absolutely right. My bad. With your proposed implementation:
template
const T moveToConst(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...
const QVector getVector();
for (auto : 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,
--
Giuseppe D'Angelo | giuseppe.dang...@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
smime.p7s
Description: Firma crittografica S/MIME
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den sön 21 okt. 2018 kl 16:15 skrev Elvis Stansvik :
>
> Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
> :
> >
> > Il 20/10/18 14:43, Elvis Stansvik ha scritto:
> > > In our application we added a helper like
> > >
> > > template
> > > const T moveToConst(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 v = getVector();
> > for (auto : v) ~~~
> >
> > vs.
> >
> > // Always 2 moves
> > for (auto : 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@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@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
> #include
> #include
> #include
>
> class FooPrivate : public QSharedData {
> public:
> FooPrivate() {
> qDebug() << "FooPrivate default constr";
> };
>
> FooPrivate(const FooPrivate ) : QSharedData(other) {
> qDebug() << "FooPrivate copy constr";
> };
>
> explicit FooPrivate(const QVector ) : v(v) {
> qDebug() << "FooPrivate constr from vector";
> };
>
> ~FooPrivate() {
> qDebug() << "FooPrivate destr";
> };
>
> QVector v;
> };
>
> class Foo {
> public:
> Foo() : d(new FooPrivate) {
> qDebug() << "Foo default constr" << this;
> };
>
> Foo(const Foo ) : d(other.d) {
> qDebug() << "Foo copy constr" << this;
> };
>
> Foo(Foo &) : d(other.d) {
> other.d = nullptr;
> qDebug() << "Foo move constr" << this;
> };
>
> explicit Foo(const QVector ) : d(new FooPrivate(v)) {
> qDebug() << "Foo constr with arg" << this;
> };
>
> ~Foo() {
> qDebug() << "Foo destr" << this;
> };
>
> Foo =(const Foo ) {
> qDebug() << "Foo copy ass op" << this;
> d = other.d;
> return *this;
> };
>
> QVector::iterator begin() {
> qDebug() << "Foo begin" << this;
> return d->v.begin();
> };
>
> QVector::const_iterator begin() const {
> qDebug() << "Foo begin const" << this;
> return d->v.begin();
> };
>
>
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
:
>
> Il 20/10/18 14:43, Elvis Stansvik ha scritto:
> > In our application we added a helper like
> >
> > template
> > const T moveToConst(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 v = getVector();
> for (auto : v) ~~~
>
> vs.
>
> // Always 2 moves
> for (auto : 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:
[estan@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@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
#include
#include
#include
class FooPrivate : public QSharedData {
public:
FooPrivate() {
qDebug() << "FooPrivate default constr";
};
FooPrivate(const FooPrivate ) : QSharedData(other) {
qDebug() << "FooPrivate copy constr";
};
explicit FooPrivate(const QVector ) : v(v) {
qDebug() << "FooPrivate constr from vector";
};
~FooPrivate() {
qDebug() << "FooPrivate destr";
};
QVector v;
};
class Foo {
public:
Foo() : d(new FooPrivate) {
qDebug() << "Foo default constr" << this;
};
Foo(const Foo ) : d(other.d) {
qDebug() << "Foo copy constr" << this;
};
Foo(Foo &) : d(other.d) {
other.d = nullptr;
qDebug() << "Foo move constr" << this;
};
explicit Foo(const QVector ) : d(new FooPrivate(v)) {
qDebug() << "Foo constr with arg" << this;
};
~Foo() {
qDebug() << "Foo destr" << this;
};
Foo =(const Foo ) {
qDebug() << "Foo copy ass op" << this;
d = other.d;
return *this;
};
QVector::iterator begin() {
qDebug() << "Foo begin" << this;
return d->v.begin();
};
QVector::const_iterator begin() const {
qDebug() << "Foo begin const" << this;
return d->v.begin();
};
QVector::const_iterator cbegin() const {
qDebug() << "Foo cbegin" << this;
return d->v.cbegin();
};
QVector::iterator end() {
qDebug() << "Foo end" << this;
return d->v.end();
};
QVector::const_iterator end() const {
qDebug() << "Foo end const" << this;
return d->v.end();
};
QVector::const_iterator cend() {
qDebug() << "Foo cend" << this;
return d->v.cend();
};
private:
QSharedDataPointer d;
};
template
const T moveToConst(T
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
(Adding back the mailing list)
Den lör 20 okt. 2018 kl 23:54 skrev Elvis Stansvik :
>
> Den lör 20 okt. 2018 kl 23:50 skrev Giuseppe D'Angelo
> :
> >
> > Il 20/10/18 19:37, Elvis Stansvik ha scritto:
> > > 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 (?).
> >
> > Probably in the sense that the function you pasted can be applied like this:
> >
> > QVector v;
> > for (auto : 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 & : as_const(getQString())) // whoops!
{
}
Elvis
>
> >
> >
> > > What's GCE? Some optimization? (Google "c++ gce" didn't give me anything).
> >
> > Guaranteed Copy Elision:
> >
> > > https://en.cppreference.com/w/cpp/language/copy_elision
>
> Thanks!
>
> Elvis
>
> >
> > My 2 c,
> >
> > --
> > Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Den lör 20 okt. 2018 kl 18:53 skrev Giuseppe D'Angelo via Development
:
>
> Il 20/10/18 14:43, Elvis Stansvik ha scritto:
> > In our application we added a helper like
> >
> > template
> > const T moveToConst(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 v = getVector();
> for (auto : v) ~~~
>
> vs.
>
> // Always 2 moves
> for (auto : 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,
> --
> Giuseppe D'Angelo | giuseppe.dang...@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@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] qMoveToConst helper for rvalue references to movable Qt containers?
Il 20/10/18 14:43, Elvis Stansvik ha scritto:
In our application we added a helper like
template
const T moveToConst(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 v = getVector();
for (auto : v) ~~~
vs.
// Always 2 moves
for (auto : 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?
My 2 c,
--
Giuseppe D'Angelo | giuseppe.dang...@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
smime.p7s
Description: Firma crittografica S/MIME
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
[Development] qMoveToConst helper for rvalue references to movable Qt containers?
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
const T moveToConst(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
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
