Re: [Development] FileRe: Move ctors for q_declare_shared types
If a change is related to a discussion on the mailing list, I expect that the change is posted to the discussion. I already quoted you the mail where I ... Because not doing that, circumvents the discussion on the mailing list and goes against the spirit of The Qt Governance Model. You circumvented the discussion and did not inform the mailing list on the changes, which clearly were related: https://codereview.qt-project.org/115376 ... shared this ^ change on the ML. Now you _again_ claim I didn't. I never claimed that you didn't share this change. Also I mentioned that change exactly once so far. Can you please not grossly misrepresent what I actually wrote? I wrote and this is from the only mail in which I mention that change, and I hope you read it this time: You circumvented the discussion and did not inform the mailing list on the changes: You only informed the mailing list on that change after it was already in the repository. You didn't wait until the discussion came to a conclusion. If you would have posted that change for discussion, I would have pointed out the how broken that change is without a change to the container classes. I stand by my point, you circumvented the discussion on the mailing list. https://codereview.qt-project.org/#/c/120771 https://codereview.qt-project.org/#/c/120804/ These just implement what was discussed on the ML. Again: no-one spoke out against the solution Thiago and I came up with (publicly on the ML). I posted one change, I didn't post them all, no. They contain no new information. I was under the impression that you wanted to not fix the container classes at all. Can you quote me the mail on the mailing list where you or Thiago propose and agree on fixing the container classes? I have a few dozen of changes (re)adding move semantics to Qt types. Do you want to see them all? I can CC you from now on on Gerrit. I doubt everyone else will want to be spammed on the ML. Again, and I'm not sure why this is complicated for you to understand: A change that is related to a ongoing discussion on the mailing list, should be discussed on the mailing list. You shouldn't create hard facts by pushing a change. In my opinion this is unprofessional. Then I guess I'm unprofessional. You got what you lobbied for. What, exactly, are you complaining about? Your unprofessional behaviour. I thought I made clear that I'm fine with the fixes in Qt. daniel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
Are we going to announce every API change on the ML now? No, that is obviously stupid. But, could you please stop trying to divert a discussion by raising unrelated questions? If a change is related to a discussion on the mailing list, I expect that the change is posted to the discussion. Because not doing that, circumvents the discussion on the mailing list and goes against the spirit of The Qt Governance Model. You circumvented the discussion and did not inform the mailing list on the changes, which clearly were related: https://codereview.qt-project.org/115376 https://codereview.qt-project.org/#/c/120771 https://codereview.qt-project.org/#/c/120804/ In my opinion this is unprofessional. daniel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
On Wednesday 22 July 2015 12:07:06 Daniel Teske wrote: Are we going to announce every API change on the ML now? No, that is obviously stupid. But, could you please stop trying to divert a discussion by raising unrelated questions? If a change is related to a discussion on the mailing list, I expect that the change is posted to the discussion. I already quoted you the mail where I ... Because not doing that, circumvents the discussion on the mailing list and goes against the spirit of The Qt Governance Model. You circumvented the discussion and did not inform the mailing list on the changes, which clearly were related: https://codereview.qt-project.org/115376 ... shared this ^ change on the ML. Now you _again_ claim I didn't. https://codereview.qt-project.org/#/c/120771 https://codereview.qt-project.org/#/c/120804/ These just implement what was discussed on the ML. Again: no-one spoke out against the solution Thiago and I came up with (publicly on the ML). I posted one change, I didn't post them all, no. They contain no new information. I have a few dozen of changes (re)adding move semantics to Qt types. Do you want to see them all? I can CC you from now on on Gerrit. I doubt everyone else will want to be spammed on the ML. In my opinion this is unprofessional. Then I guess I'm unprofessional. You got what you lobbied for. What, exactly, are you complaining about? Thanks, Marc -- Marc Mutz marc.m...@kdab.com | Senior Software Engineer KDAB (Deutschland) GmbH Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
Hi,
https://codereview.qt-project.org/#/c/120804/ and
https://codereview.qt-project.org/#/c/120771 do fix the move assignment
operators in Qt, just like I wanted.
I wonder why Marc Mutz didn't fell the need to update the mailing list on his
changed opinion and this fix.
Anyway, for those who don't want to read the full thread but want to know how
write a move assignment operator. This advice by Howard E. Hinnant is good:
In general a move assignment operator should:
- Destroy visible resources (though maybe save implementation detail
resources).
- Move assign all bases and members.
- If the move assignment of bases and members didn't make the rhs resource-
less, then make it so.
In code this looks e.g. like this:
QVectorT operator=(QVectorT other) Q_DECL_NOTHROW
{ QVector moved(std::move(other)); swap(moved); return *this; }
Now, if and only if the destructor of the class has no side effects or those
side effects are inconsequential, you might consider implementing move
assignment as a swap between other and this.
But if you are holding a user defined type or if you are holding resources like
file handlers or network sockets, you shouldn't implement move assignment via a
simple swap between other and this.
As a counter example, std::string's move assignment operator is allowed to be
implemented as a simple swap.
daniel
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
On Tuesday 21 July 2015 18:11:20 Daniel Teske wrote: I wonder why Marc Mutz didn't fell the need to update the mailing list on his changed opinion and this fix. Read the whole mail you're replying to, and the follow-ups: On Monday 29 June 2015 17:50:31 Thiago Macieira wrote: On Monday 29 June 2015 17:14:57 Marc Mutz wrote: So, it doubles the number of dtor calls, and it pessimises all code around it. I am not prepared to make that pessimisation for out-of-line types. People who use std::move can imo be bothered to clear the moved-from object after the move in those miniscule fraction of cases where that actually matters. Forcing people to use swap (if, indeed, they can, because swap() doesn't bind to rvalues on the rhs) for the common case is bad api design. I agree with Marc here. [...] For inline types, see https://codereview.qt-project.org/115376 Nobody spoke up again, so that's what I implemented and announced in the [ChangeLog] here: https://codereview.qt-project.org/120771 Are we going to announce every API change on the ML now? Thanks, Marc -- Marc Mutz marc.m...@kdab.com | Senior Software Engineer KDAB (Deutschland) GmbH Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
On Monday 29 June 2015 13:44:13 Daniel Teske wrote: Instead I will argue that: This guarantee is important, valuable and good and Qt not giving this is bad. So, in this example: QSharedPointerSomeClass a = [...] QSharedPointerSomeClass b = [...] a = std::move(b); I repect Howard a lot, and he's right, to a point, even though I'd argue that explicitly clearing a moved-from object that isn't immediately going out of scope afterwards should be considered good coding practice, if only to give the reader of the code a hint as to what the state of the object is now. But there are additional constraints here. The std library contains next to no out-of-line classes. Destoying the LHS upon move assignment is thus trivial, and comes at only minor cost (though the O(N) behaviour _can_ be troublesome in practice even so). Consequently, all your examples are of template classes. But for most Qt types, and indeed the vast majority of those this thread is^Wwas originally about, there is an extremly high cost associated with destroying the LHS compared to just swapping the state into the RHS: out-of- line destructor calls. They are not free. They act as compiler-firewalls. The compiler loses a lot of the little information about a pimpled object that it had, and not only that: it loses information about at the very least all other objects of the same type in scope, because it must assume that they are all shared. It probably loses *all* information about *all* reference type objects in scope, too. That needs to be seen in connection with the future of the RHS. In the vast majority of cases, the RHS just gets its dtor called at the very next opportunity. If the LHS was destroyed as part of the move, then the RHS dtor will have little to nothing to do. It's an extremely expensive no-op, and doubles the number of dtor calls compared to swapping. So, it doubles the number of dtor calls, and it pessimises all code around it. I am not prepared to make that pessimisation for out-of-line types. People who use std::move can imo be bothered to clear the moved-from object after the move in those miniscule fraction of cases where that actually matters. Forcing people to use swap (if, indeed, they can, because swap() doesn't bind to rvalues on the rhs) for the common case is bad api design. For inline types, see https://codereview.qt-project.org/115376 Thanks, Marc -- Marc Mutz marc.m...@kdab.com | Senior Software Engineer KDAB (Deutschland) GmbH Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] FileRe: Move ctors for q_declare_shared types
- QVector should give the same guarantee as the standard containers. I disagree with that. QVector is not std::vector. At first, it is implicitly shared, so that's already a big difference. And therefore we can allow ourselfs many more differences. But this point still stands. Right, there's a argument possible that gratuitous differences should be avoided, after all it has the same name, so it should be semantically close, but well, I'm not going to bother with that. Instead I will argue that: This guarantee is important, valuable and good and Qt not giving this is bad. So, in this example: QSharedPointerSomeClass a = [...] QSharedPointerSomeClass b = [...] a = std::move(b); after this line, b holds a reference to whatever a contained prior to the move, since QSharedPointer is using swap to implement move assignment. This is (mostly) fine if b is a temporary, since it only affects the order of destruction. If b though is a e.g. a member variable of a class, and the user intends to move resources between objects, b might not be destructed until much later. If the order of destruction is important, or that a resource destruction has a side effect this is bad. Since Qt can not know whether the destructor of the type held in a shared pointer or a vector has a important side effect, Qt classes have to assume that there are such side effects. It is also surprising and counter intuitive that copy assignment and move assignment treat the old value differently, and none of the standard library types that hold user types behave in such a way. If the user intended this behavior, s/he could have called swap instead. In support of this argument, let me point to the history of this guarantee for vector in the standard. (The text for shared_ptr always contained the guarantee, so the history there is not as interesting.) First, in 2007!, the issue 675 added the relevant text: All existing elements of a are either move assigned or destructed. was added to container.requirements. See Issue 675 at http://open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2456.html. Do note, that the proposed resolution is at the bottom, and the discussion is best understood with the proposed resolution in mind. This issue and proposed resolution was submitted by Howard E. Hinnant, who was the lead designer and author of the move semantics standardization papers, is the lead author of libc++ and was the chairman of the LWG from 2005 to 2010. The relevant quote: Move semantics means not caring what happens to the source (v2 in this example). It doesn't mean not caring what happens to the target (v1). In the above example both [copy and move] assignments should have the same effect on v1. Any non-shared ostream's v1 owns before the assignment should be closed, whether v1 is undergoing copy assignment or move assignment. This implies that the semantics of move assignment of a generic container should be clear, swap instead of just swap. An alternative which could achieve the same effect would be to move assign each element. In either case, the complexity of move assignment needs to be relaxed to O(v1.size()). The same argument of course applies to QSharedPointer too: QSharedPointerQFile a; QSharedPointerQFile b; a = std::move(b) should close the old QFile held in a. And here: http://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments Howard E. Hinnant writes: In general a move assignment operator should: - Destroy visible resources (though maybe save implementation detail resources). - Move assign all bases and members. - If the move assignment of bases and members didn't make the rhs resource- less, then make it so. daniel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] FileRe: Move ctors for q_declare_shared types
On Monday 29 June 2015 17:14:57 Marc Mutz wrote:
So, it doubles the number of dtor calls, and it pessimises all code around
it. I am not prepared to make that pessimisation for out-of-line types.
People who use std::move can imo be bothered to clear the moved-from object
after the move in those miniscule fraction of cases where that actually
matters. Forcing people to use swap (if, indeed, they can, because swap()
doesn't bind to rvalues on the rhs) for the common case is bad api design.
I agree with Marc here.
But, isn't there a third way for some types? For those that can take a null d-
pointer, it should be easy to just do:
Klass operator=(Klass other)
{ d = other.d; other.d = nullptr; return *this; }
This is better than the three-way swap because there's no extra temporary
being constructed and destructed out-of-line.
The drawback is that it implies null-d-pointer support forever, since it got
inlined in user code.
--
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] FileRe: Move ctors for q_declare_shared types
On Monday 29 June 2015 20:42:40 Marc Mutz wrote:
But, isn't there a third way for some types? For those that can take a
null
d- pointer, it should be easy to just do:
Klass operator=(Klass other)
{ d = other.d; other.d = nullptr; return *this; }
Doesn't work. this-d is leaked in the process.
Never mind then...
--
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] FileRe: Move ctors for q_declare_shared types
On Monday 29 June 2015 17:50:31 Thiago Macieira wrote:
On Monday 29 June 2015 17:14:57 Marc Mutz wrote:
So, it doubles the number of dtor calls, and it pessimises all code
around it. I am not prepared to make that pessimisation for out-of-line
types. People who use std::move can imo be bothered to clear the
moved-from object after the move in those miniscule fraction of cases
where that actually matters. Forcing people to use swap (if, indeed,
they can, because swap() doesn't bind to rvalues on the rhs) for the
common case is bad api design.
I agree with Marc here.
But, isn't there a third way for some types? For those that can take a null
d- pointer, it should be easy to just do:
Klass operator=(Klass other)
{ d = other.d; other.d = nullptr; return *this; }
Doesn't work. this-d is leaked in the process.
--
Marc Mutz marc.m...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - The Qt Experts
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
