Treeitem(Model): Fixes & Optimizations

[uklotzde]

...using std::unique_ptr for transfering ownership.

I also improved the distinction between a label (QString) and the data (QVariant) of a TreeItem. The code contained unnecessary QString <-> int conversions for the data of some tree items.

Still much room for improvements, but that should be enough for now. I just need this as a prerequisite for https://github.com/uklotzde/mixxx/tree/cratestorage.

[daschuer]

I am just thinking about the best way to pass the unique_ptr here.

I did expect that it has to be "std::unique_ptr&& pRootItem" just as like a move constructor.
After reading a bit the current implementation seams also common, but why?

setRootItem(std::move(root));
std::move casts root to an rvalue reference. In the pass by value case, the rvalue reference is moved to the function parameter, emptying "root". Inside setRootItem() the same happens again, cast to rvalue, copy rvalue to value = move.

If we switch to a pass by rvalue reference. We only have a single move action inside setRootItem.
From the interface the caller has no idea if the move actually happens or not.
As have read, this is considered as disadvantage.
I do not understand why this is important. This is the same when using move constructors.
I would just consider a value that was used by std::move() as invalid.

How do you think about it?

[daschuer]

I have just tested it and it works nice.

[uklotzde]

Passing a std::unique::ptr by value means passing ownership. This is the main purpose of unique_ptr, it only has a move constructor and move assignment operator. The smart pointer introduces an extra level of indirection, that avoids passing r-value references explicitly. There is a small overhead, but providing both && and const& variants for ordinary functions is simply too complicated and would require multiple variants of each function. Prefer to pass move-constructible parameters by value, if you need to copy the parameter. Then the caller is able to decide if it is able to use std:move() or actually pass a copy. This depends on the caller's context, because std::move() invalidates the argument on the caller side! Only the caller is able to decide if this is possible.

…

On 13.12.2016 23:43, Daniel Schürmann wrote: ***@***.**** commented on this pull request. ---------------------------------------------------------------------------- In src/library/treeitemmodel.cpp <#1067 (review)>: > } /** * Populates the model and notifies the view. * Call this method first, before you do call any other methods. */ -void TreeItemModel::setRootItem(TreeItem *item) { - if(m_pRootItem) delete m_pRootItem; - - m_pRootItem = item; +TreeItem* TreeItemModel::setRootItem(std::unique_ptr<TreeItem> pRootItem) { I am just thinking about the best way to pass the unique_ptr here. I did expect that it has to be "std::unique_ptr&& pRootItem" just as like a move constructor. After reading a bit the current implementation seams also common, but why? setRootItem(std::move(root)); std::move casts root to an rvalue reference. In the pass by value case, the rvalue reference is moved to the function parameter, emptying "root". Inside setRootItem() the same happens again, cast to rvalue, copy rvalue to value = move. If we switch to a pass by rvalue reference. We only have a single move action inside setRootItem. >From the interface the caller has no idea if the move actually happens or not. As have read, this is considered as disadvantage. I do not understand why this is important. This is the same when using move constructors. I would just consider a value that was used by std::move() as invalid. How do you think about it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1067 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZ-anb2acQ5WSizSqIalYEcPasr6j_Kks5rHx-ugaJpZM4LIW1P>.

[daschuer]

First of all, both versions are working for me. I just want to learn and verify your way of thinking with mine.

I understand that passing the parameter by value is a nice universal rule of thumb, since it allows to use always std:move instead of copy inside the function (moving from the parameter on the private stack) .

The situation here is different, since std::unique_ptr force to get an rvalue reference every time.
There is no need for the intermediate value on the private function stack.
IMHO a && parameter also clarifies the intention of the API best. "Give me an rvalue! I may keep it".
It fails, if you do not use std:move(). This is just the way a move constructor behaves.

Passing a std::unique::ptr by value means passing ownership.

Passing an rvalue reference may pass the ownership as well, we have just no guarantee without knowing the implementation details.
Is that Important? In both cases the std:move() on the caller side indicates that the object can be invalid. If we assume the called function does not takes the ownership. The object is destroyed at the end of the called function in the value case and at the end of the caller function in the && case.

The smart pointer introduces an extra level of indirection, that avoids passing r-value references explicitly.

What do you mean? I can compile the passing by rvalue reference version and it works.

[uklotzde]

*F.26: Use a unique_ptr<T> to transfer ownership where a pointer is needed* https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-unique_ptr *Pass unique_ptr<T> by value to claim/transfer ownership* http://stackoverflow.com/questions/8114276/how-do-i-pass-a-unique-ptr-argument-to-a-constructor-or-a-function Using an r-value reference may lead to unexpected behavior / side effects if the called function fails to take ownership by not storing the passed r-value!

…

On 14.12.2016 11:40, Daniel Schürmann wrote: First of all, both versions are working for me. I just want to learn and verify your way of thinking with mine. I understand that passing the parameter by value is a nice universal rule of thumb, since it allows to use always std:move instead of copy inside the function (moving from the parameter on the private stack) . The situation here is different, since std::unique_ptr force to get an rvalue reference every time. There is no need for the intermediate value on the private function stack. IMHO a && parameter also clarifies the intention of the API best. "Give me an rvalue! I may keep it". It fails, if you do not use std:move(). This is just the way a move constructor behaves. Passing a std::unique::ptr by value means passing ownership. Passing an rvalue reference may pass the ownership as well, we have just no guarantee without knowing the implementation details. Is that Important? In both cases the std:move() on the caller side indicates that the object can be invalid. If we assume the called function does not takes the ownership. The object is destroyed at the end of the called function in the value case and at the end of the caller function in the && case. The smart pointer introduces an extra level of indirection, that avoids passing r-value references explicitly. What do you mean? I can compile the passing by rvalue reference version and it works. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZ-as1-KMMQeGpbDbX-_Pu7Bbv9esDiks5rH8ekgaJpZM4LIW1P>.

[daschuer]

Thank you for the links.

Unfortunately the excepted answer does not answer the question, why it is a problem, that a && parameter function may not take the ownership? IMHO there is non since nothing bad happens the destructor is called as usual when the value falls out of scope.

Interesting is also this comment:

...(3) Although your argument favoring passing by value over passing by rvalue reference makes sense, I think that the standard itself always passes unique_ptr values by rvalue reference (for instance when transforming them into shared_ptr). The rationale for that might be that it is slightly more efficient (no moving to temporary pointers is done) while it gives the exact same rights to the caller (may pass rvalues, or lvalues wrapped in std::move, but not naked lvalues). – Marc van Leeuwen Jun 22 '14 at 18:58

This rule C++ guidline rules also has nothing against it:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope

A remaining question I wanted to know: Is there really a performance penalty or is the compiler smart enough to optimize it away?

So I have compared the optimized assembler code from Mixxx.
It Is hard to compare, but If I get it correct, in autodjfeature.S the same calls are made in both versions. There is no additonal move visible. Hoverer, the unique_ptr destructor is called immediately after the setRootItem(value) call, while in the && case it is called at the very end. treeitemmodel.S does not change at all.

Nice: the compiler does it right.

Conclusion:
Both versions are equal fast. Since the value version works best in the general case it is a good choice here.

[uklotzde]

When using an r-value reference std::unique_ptr<T>&& it depends on the implementation of the function if the parameter passed with std::move() is still valid after the function has returned! Passing by value guarantees that the argument becomes invalid after the function call returned, independent of what actually happend inside the function. You are able to reason about the post-condition just by looking at the function definition. In this case I prefer safety and predictability over efficiency. Thanks for analyzing the generated code, Daniel! Now we even know that we don't sacrifice efficiency when using the safe version ;)

…

On 14.12.2016 16:34, Daniel Schürmann wrote: Thank you for the links. Unfortunately the excepted answer does not answer the question, why it is a problem, that a && parameter function may not take the ownership? IMHO there is non since nothing bad happens the destructor is called as usual when the value falls out of scope. Interesting is also this comment: ...(3) Although your argument favoring passing by value over passing by rvalue reference makes sense, I think that the standard itself always passes unique_ptr values by rvalue reference (for instance when transforming them into shared_ptr). The rationale for that might be that it is slightly more efficient (no moving to temporary pointers is done) while it gives the exact same rights to the caller (may pass rvalues, or lvalues wrapped in std::move, but not naked lvalues). – Marc van Leeuwen Jun 22 '14 at 18:58 This rule C++ guidline rules also has nothing against it: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope A remaining question I wanted to know: Is there really a performance penalty or is the compiler smart enough to optimize it away? So I have compared the optimized assembler code from Mixxx. It Is hard to compare, but If I get it correct, in autodjfeature.S the same calls are made in both versions. There is no additonal move visible. Hoverer, the unique_ptr destructor is called immediately after the setRootItem(value) call, while in the && case it is called at the very end. treeitemmodel.S does not change at all. Nice: the compiler does it right. Conclusion: Both versions are equal fast. Since the value version works best in the general case it is a good choice here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZ-at1_llQ0AHxXE_PGKhw2Jbqv29V8ks5rIAx7gaJpZM4LIW1P>.

[daschuer]

LGTM! Thank you.