<div dir="ltr"><div>Hi, Stefano!<br><br></div>I agree with most of your concerns, though I feel a bit of skeptical about some of the points, especially in the solution part.<br><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>1) They lead to subtle and hard to resolve memory leaks.  <br></div></div></blockquote><div> </div><div><br></div><div>Well, yes. This is the most troublesome part. And yes, we should solve this problem, because this is one of the reasons why KisTileDataPool sometimes cannot free the memory chunks on closing the image. Though I feel that the solution should be a bit different, not raw pointers, but weak shared pointers.<br></div><div dir="ltr"> <br><div style="font-size:13px">  | 2) They lead to hard to debug malfunctions of the code.</div><div style="font-size:13px"><br>This is easy to explain, a class that stays alive beyond what one thought it would, could receive function calls that accesses it or other classes, making it do things it shouldn't at that point in time.<br><br></div><div style="font-size:13px">Right now you are most probably talking about our dockers. The solution here is either to try to fix the design of the part that handles UI and dockers or use weak shared pointers. *But* here we need shared pointers in terms if Qt, not Krita. <br></div><div style="font-size:13px"><br></div><div style="font-size:13px">   | 3) They introduce hard to solve crashes.<br></div><div style="font-size:13px"><br>This is rather rare. Most of the time the code that owns a dangling shared pointer it runs "somehow" and at least doesn't crash :)<br></div><div style="font-size:13px"><br></div><div style="font-size:13px">   |  4) They add distributed overhead that is hard to pinpoint.</div><br></div><div>Well,  to tell you the truth I never saw shared pointers being a problem in the dumps of Valgrind and VTune. And what is more, we use shared pointers for storing the tiles inside a tile engine and they don't seem to appear in dums as well. So even if it creates much overhead, then it is really-really distributed somewhere in the memory bandwidth, that hides in all the logs.<br></div><div dir="ltr"><br><blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">What i'm proposing:<div style="font-size:13px">...<br></div> In general though the shared pointers, wherever they are, shouldn't be passed down the stack, but up.</blockquote></blockquote><div>Well, I cannot agree with it. Passing a raw pointer down the stack and acquiring it somewhere below will make the code crashy and really hard to understand why. Who would guarantee how the passed object will be used inside the function? Will it be stored or not? How the caller has allocate and stored the pointer? Does he own it or not? There might be really funny situations, like:<br><br><div>struct SomeTempStruct {<br>    SomeTempStruct(KisNode *node) : m_node(node) {}<br></div><div>    KisNodeSP m_node;<br><br></div><div>    void bar() { /* do something */  }<br></div>};<br><br></div><div>void foo(KisNode* node) {<br></div><div>    SomeTempStruct myStruct(node);<br></div><div>    myStruct.bar();<br></div><div>}<br></div><div> <br></div><div>KisNode *node = new KisNode();<br></div><div>foo(node);<br></div><div>delete node; // <-- double-delete crash here<br><br></div><div>This nice and quite expected code will crash happily :)<br></div><div><br></div><div>So the general idea is: "one should not mix raw and shared pointers even when the counter is stored inside the object itself". Of course, "...when possible".<br></div><div><br></div><div>From the four shared pointers drawbacks you listed I can fully agree only with the first one. Most of the leaks happen when the dockers or some UI managers store a shared pointer to the painting object. This can be fixed by obliging the UI not to store shared pointers, but store weak shared pointers instead. Of course, when possible. The idea provided by Qt's shared pointers might be very helpful here (Qt's weak shared pointer doesn't allow dereferencing the pointer until upgrade to a real shared pointer). Though for catching design bugs (in form of crashes) our weak shared pointers might also be used.<br><br></div><div>So as a conclusion for my long mail I would suggest to consider the following:<br><br></div><div>1) Use weak shared pointers in UI dockers or elements.<br></div><div>2) Upgrade weak shared pointer to real pointer before use in UI (?)<br></div><div>3) Don't use manual deletion of Private classes. Use QScopedPointer<Private> instead. This is the most popular cause of memory leaks and dangling shared pointer in Krita, btw. It also guarantees the "c++ rule of three" is satisfied properly (it disables default copy-ctor by default).<br></div><div><br></div></div></div><br clear="all"><br>-- <br><div class="gmail_signature">Dmitry Kazakov</div>
</div></div>